# HG changeset patch # User Asier Lostalé # Date 1426166760 -3600 # Thu Mar 12 14:26:00 2015 +0100 # Node ID d8a10c0aeb51413ff4864d3095385d24c1965b7d # Parent 5cc98c60fb3c02ccebf85a2ab28aae0812b0b93f related to bug 29258: LockOptions can be set to OBQuery This allows to define OBQueries to acquire a DB lock (ie. select for update) diff -r 5cc98c60fb3c -r d8a10c0aeb51 src/org/openbravo/dal/service/OBQuery.java --- a/src/org/openbravo/dal/service/OBQuery.java Tue Mar 10 13:01:12 2015 +0100 +++ b/src/org/openbravo/dal/service/OBQuery.java Thu Mar 12 14:26:00 2015 +0100 @@ -11,7 +11,7 @@ * under the License. * The Original Code is Openbravo ERP. * The Initial Developer of the Original Code is Openbravo SLU - * All portions are Copyright (C) 2008-2014 Openbravo SLU + * All portions are Copyright (C) 2008-2015 Openbravo SLU * All Rights Reserved. * Contributor(s): ______________________________________. ************************************************************************ @@ -28,6 +28,7 @@ import org.apache.log4j.Logger; import org.hibernate.HibernateException; +import org.hibernate.LockOptions; import org.hibernate.Query; import org.hibernate.ScrollMode; import org.hibernate.ScrollableResults; @@ -76,6 +77,8 @@ private String selectClause; + private LockOptions lockOptions; + // package visible OBQuery() { } @@ -232,6 +235,9 @@ try { final Query qry = getSession().createQuery(qryStr); setParameters(qry); + if (lockOptions != null) { + qry.setLockOptions(lockOptions); + } if (fetchSize > -1) { qry.setFetchSize(fetchSize); } @@ -650,4 +656,18 @@ public String getQueryType() { return this.queryType; } + + /** + * Define LockOptions for current query. {@link LockOptions#UPGRADE} can be used to force a lock + * at DB level (select for update) will be executed. + *

+ * Note in case of queries with joins {@link LockOptions#UPGRADE} will create locks on ALL rows + * participating in the lock. In this case it is possible to create a LockOptions specifying the + * aliases in the query to lock. + * + * @see LockOptions + */ + public void setLockOptions(LockOptions lockOptions) { + this.lockOptions = lockOptions; + } } # HG changeset patch # User Asier Lostalé # Date 1426167934 -3600 # Thu Mar 12 14:45:34 2015 +0100 # Node ID 90944b976b2da0ce70df66041cd223e279490267 # Parent d8a10c0aeb51413ff4864d3095385d24c1965b7d fixed bug 29258: FIN_Utility.getDocumentNo method can return duplicates -Fixes the way lock is got in case docType.getDocumentSequence() is not null, previous code tried to do it but it didn't work due to a Hibernate issue [1] -Acquires lock also when docType.getDocumentSequence() is null [1] https://hibernate.atlassian.net/browse/HHH-5275 diff -r d8a10c0aeb51 -r 90944b976b2d modules/org.openbravo.advpaymentmngt/src/org/openbravo/advpaymentmngt/utility/FIN_Utility.java --- a/modules/org.openbravo.advpaymentmngt/src/org/openbravo/advpaymentmngt/utility/FIN_Utility.java Thu Mar 12 14:26:00 2015 +0100 +++ b/modules/org.openbravo.advpaymentmngt/src/org/openbravo/advpaymentmngt/utility/FIN_Utility.java Thu Mar 12 14:45:34 2015 +0100 @@ -37,7 +37,7 @@ import org.apache.commons.lang.StringUtils; import org.apache.commons.lang.time.DateUtils; -import org.hibernate.LockMode; +import org.hibernate.LockOptions; import org.hibernate.Query; import org.hibernate.Session; import org.hibernate.criterion.Restrictions; @@ -351,14 +351,35 @@ String nextDocNumber = ""; if (docType != null) { Sequence seq = docType.getDocumentSequence(); - if (seq == null && tableName != null) { - OBCriteria obcSeq = OBDal.getInstance().createCriteria(Sequence.class); - obcSeq.add(Restrictions.eq(Sequence.PROPERTY_NAME, tableName)); - obcSeq.setLockMode(LockMode.PESSIMISTIC_WRITE); - if (obcSeq != null && obcSeq.list().size() > 0) { - seq = obcSeq.list().get(0); + if (seq != null) { + // Force seq to be reloaded from db with LockOptions.UPGRADE to acquire a lock (select ... + // from ad_sequence for update) in order to prevent getting a duplicate document number, see + // issue #29258. Note in case another thread already took this lock this one will wait until + // it is released + long t = System.currentTimeMillis(); + OBQuery qSeq = OBDal.getInstance().createQuery(Sequence.class, "id=:id"); + qSeq.setNamedParameter("id", DalUtil.getId(seq)); + qSeq.setLockOptions(LockOptions.UPGRADE); + seq = qSeq.uniqueResult(); + log4j + .debug("lock on ad_sequnce acquired after " + (System.currentTimeMillis() - t) + " ms"); + } else if (tableName != null) { + // For seq to be loaded from db with LockOptions.UPGRADE to acquire a lock (select ... + // from ad_sequence for update) in order to prevent getting a duplicate document number, see + // issue #29258. Note in case another thread already took this lock this one will wait until + // it is released + long t = System.currentTimeMillis(); + OBQuery qSeq = OBDal.getInstance().createQuery(Sequence.class, + Sequence.PROPERTY_NAME + "=:name"); + qSeq.setNamedParameter("name", tableName); + qSeq.setLockOptions(LockOptions.UPGRADE); + if (qSeq.list().size() > 0) { + seq = qSeq.list().get(0); } + log4j + .debug("lock on ad_sequnce acquired after " + (System.currentTimeMillis() - t) + " ms"); } + if (seq != null) { if (seq.getPrefix() != null) nextDocNumber = seq.getPrefix(); # HG changeset patch # User Asier Lostalé # Date 1426169011 -3600 # Thu Mar 12 15:03:31 2015 +0100 # Node ID 8f74ad66220711b75c7c387e43028f09952a6213 # Parent 90944b976b2da0ce70df66041cd223e279490267 related to bug 29258: added test cases to ensure doc no uniquess generation diff -r 90944b976b2d -r 8f74ad662207 src-test/src/org/openbravo/test/AllAntTaskTests.java --- a/src-test/src/org/openbravo/test/AllAntTaskTests.java Thu Mar 12 14:45:34 2015 +0100 +++ b/src-test/src/org/openbravo/test/AllAntTaskTests.java Thu Mar 12 15:03:31 2015 +0100 @@ -44,6 +44,7 @@ import org.openbravo.test.dal.ReadByNameTest; import org.openbravo.test.dal.ValidationTest; import org.openbravo.test.dal.ViewTest; +import org.openbravo.test.documents.DocumentNumberGeneration; import org.openbravo.test.expression.EvaluationTest; import org.openbravo.test.model.ClassLoaderTest; import org.openbravo.test.model.IndexesTest; @@ -152,6 +153,9 @@ ClassicSelectorTest.class, // Accounting - RecordID2Test.class }) + RecordID2Test.class, // + + // Basic functionallity + DocumentNumberGeneration.class }) public class AllAntTaskTests { } diff -r 90944b976b2d -r 8f74ad662207 src-test/src/org/openbravo/test/documents/DocumentNumberGeneration.java --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/src-test/src/org/openbravo/test/documents/DocumentNumberGeneration.java Thu Mar 12 15:03:31 2015 +0100 @@ -0,0 +1,202 @@ +/* + ************************************************************************* + * The contents of this file are subject to the Openbravo Public License + * Version 1.1 (the "License"), being the Mozilla Public License + * Version 1.1 with a permitted attribution clause; you may not use this + * file except in compliance with the License. You may obtain a copy of + * the License at http://www.openbravo.com/legal/license.html + * Software distributed under the License is distributed on an "AS IS" + * basis, WITHOUT WARRANTY OF ANY KIND, either express or implied. See the + * License for the specific language governing rights and limitations + * under the License. + * The Original Code is Openbravo ERP. + * The Initial Developer of the Original Code is Openbravo SLU + * All portions are Copyright (C) 2015 Openbravo SLU + * All Rights Reserved. + * Contributor(s): ______________________________________. + ************************************************************************ + */ + +package org.openbravo.test.documents; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertThat; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; +import org.openbravo.advpaymentmngt.utility.FIN_Utility; +import org.openbravo.base.secureApp.VariablesSecureApp; +import org.openbravo.dal.core.OBContext; +import org.openbravo.dal.core.SessionHandler; +import org.openbravo.dal.service.OBDal; +import org.openbravo.erpCommon.utility.Utility; +import org.openbravo.model.common.enterprise.DocumentType; +import org.openbravo.service.db.DalConnectionProvider; +import org.openbravo.test.base.OBBaseTest; + +/** + * Defines test cases to guarantee uniqueness in document number generation. + * + * There are 2 methods to obtain document numbers: Utility.getDocumentNo and + * FIN_Uilitiy.getDocumentNo; first one uses PL function to obtain it, whereas second one uses only + * DAL. + * + * In case of concurrent requests for the same document type, locks should occur to ensure the + * returned document number is unique. This locks should be seen in both directions DAL <--> PL. + * + * In case of 1st concurrent thread commits, 2nd thread should get a different doc number than first + * one, if 1st rolls back, 2nd should get same number. + * + * @author alostale + * + */ +@RunWith(Parameterized.class) +public class DocumentNumberGeneration extends OBBaseTest { + private static final String DOC_TYPE_ID = "466AF4B0136A4A3F9F84129711DA8BD3"; + private static final String TABLE_NAME = "C_Order"; + private static final int WAIT_MS = 200; + private static int threadNo = 0; + private boolean commitTrx; + + public DocumentNumberGeneration(boolean commitTrx) { + this.commitTrx = commitTrx; + } + + /** parameterize whether transaction should be committed (true) or rolled back (false) */ + @Parameters + public static Collection params() { + return Arrays.asList(new Object[][] { { true }, { false } }); + } + + /** 2 concurrent dal calls */ + @Test + public void twoDalCalls() throws InterruptedException, ExecutionException { + List threads = new ArrayList(); + threads.add(new DALDocumentNumberGetter().commitTrx(commitTrx)); + threads.add(new DALDocumentNumberGetter().waitBeforeStartMs(100)); + test(threads); + } + + /** 2 concurrent PL calls */ + @Test + public void twoPLCalls() throws InterruptedException, ExecutionException { + List threads = new ArrayList(); + threads.add(new DBDocumentNumberGetter().commitTrx(commitTrx)); + threads.add(new DBDocumentNumberGetter().commitTrx(commitTrx)); + test(threads); + } + + /** one dal, wait till it finishes, another dal call */ + @Test + public void twoDalCallsSequential() throws InterruptedException, ExecutionException { + List threads = new ArrayList(); + threads.add(new DALDocumentNumberGetter().commitTrx(commitTrx)); + threads.add(new DALDocumentNumberGetter().waitBeforeStartMs(WAIT_MS + 500)); + test(threads); + } + + /** dal and pl concurrently, dal starts */ + @Test + public void dalFirstThenPL() throws InterruptedException, ExecutionException { + List threads = new ArrayList(); + threads.add(new DALDocumentNumberGetter().commitTrx(commitTrx)); + threads.add(new DBDocumentNumberGetter().waitBeforeStartMs(100)); + test(threads); + } + + /** pl and dal concurrently, pl starts */ + @Test + public void plFirstThenDal() throws InterruptedException, ExecutionException { + List threads = new ArrayList(); + threads.add(new DALDocumentNumberGetter().commitTrx(commitTrx)); + threads.add(new DBDocumentNumberGetter().waitBeforeStartMs(100)); + test(threads); + } + + /** executes all the threads and asserts the results */ + private void test(List threads) throws InterruptedException, + ExecutionException { + ExecutorService executor = Executors.newFixedThreadPool(threads.size()); + List> r = executor.invokeAll(threads, 5, TimeUnit.HOURS); + String doc1 = r.get(0).get(); + String doc2 = r.get(1).get(); + if (commitTrx) { + assertThat(doc1, not(equalTo(doc2))); + } else { + assertThat(doc1, equalTo(doc2)); + } + } + + private abstract class DocumentNumberGetter implements Callable { + private int waitBeforeStart = 0; + private boolean commitWhenTrxDone = true; + + protected abstract String getDocumentNumber() throws Exception; + + @Override + public final String call() throws Exception { + Thread.sleep(waitBeforeStart); + System.out.println("Starting thread " + (threadNo++)); + setTestUserContext(); + long t = System.currentTimeMillis(); + String documentNo = getDocumentNumber(); + System.out.println("Thread " + threadNo + " got DocumentNo " + documentNo + " after " + + (System.currentTimeMillis() - t) + " ms"); + + // simulating now a long transaction + Thread.sleep(WAIT_MS); + + if (commitWhenTrxDone) { + SessionHandler.getInstance().commitAndClose(); + } else { + SessionHandler.getInstance().rollback(); + } + + OBDal.getInstance().getSession().disconnect(); + return documentNo; + } + + public DocumentNumberGetter waitBeforeStartMs(int wait) { + this.waitBeforeStart = wait; + return this; + } + + public DocumentNumberGetter commitTrx(boolean commit) { + this.commitWhenTrxDone = commit; + return this; + } + } + + private class DBDocumentNumberGetter extends DocumentNumberGetter { + @Override + protected String getDocumentNumber() throws Exception { + VariablesSecureApp vars = new VariablesSecureApp(OBContext.getOBContext().getUser().getId(), + OBContext.getOBContext().getCurrentClient().getId(), OBContext.getOBContext() + .getCurrentOrganization().getId()); + return Utility.getDocumentNo(OBDal.getInstance().getConnection(false), + new DalConnectionProvider(false), vars, "", TABLE_NAME, "", DOC_TYPE_ID, false, true); + } + } + + private class DALDocumentNumberGetter extends DocumentNumberGetter { + @Override + protected String getDocumentNumber() throws Exception { + return FIN_Utility.getDocumentNo(OBDal.getInstance().get(DocumentType.class, DOC_TYPE_ID), + TABLE_NAME); + } + } +}