IDEMPIERE-6036 Serial number control has potential to generate duplicate serial number under heavy load (#2236)

org.adempiere.base/src/org/compiere/model/MSerNoCtl.java

@@ -17,8 +17,12 @@
 package org.compiere.model;
 
 import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.sql.Savepoint;
 import java.util.Properties;
 
+import org.adempiere.exceptions.DBException;
+import org.compiere.util.Trx;
 import org.compiere.util.Util;
 
 /**
@@ -86,18 +90,64 @@ public class MSerNoCtl extends X_M_SerNoCtl
 	 */
 	public String createSerNo ()
 	{
-		StringBuilder name = new StringBuilder();
+		//use optimistic locking and try 3 time
-		if (getPrefix() != null)
+		set_OptimisticLockingColumns(new String[]{COLUMNNAME_CurrentNext});
-			name.append(getPrefix());
+		set_UseOptimisticLocking(true);
-		int no = getCurrentNext();
+		for(int i = 0; i < 3; i++)
-		name.append(no);
+		{
-		if (getSuffix() != null)
+			this.load(get_TrxName());
-			name.append(getSuffix());
+			//create savepoint for rollback (if in trx)
-		//
+			Trx trx = null;
-		no += getIncrementNo();
+			Savepoint savepoint = null;
-		setCurrentNext(no);
+			if (get_TrxName() != null)
-		saveEx();
+				trx = Trx.get(get_TrxName(), false);
-		return name.toString();
+			if (trx != null) {
+				try {
+					savepoint = trx.setSavepoint(null);
+				} catch (SQLException e) {
+					throw new DBException(e);
+				}
+			}
+			try {
+				StringBuilder name = new StringBuilder();
+				if (getPrefix() != null)
+					name.append(getPrefix());
+				int no = getCurrentNext();
+				name.append(no);
+				if (getSuffix() != null)
+					name.append(getSuffix());
+				//
+				no += getIncrementNo();
+				setCurrentNext(no);		
+				saveEx();
+				return name.toString();
+			} catch (RuntimeException e) {
+				if (savepoint != null) {
+					try {
+						trx.rollback(savepoint);
+					} catch (SQLException e1) {
+						throw new DBException(e1);
+					}
+					savepoint = null;
+				}
+				if (i == 2)
+					throw e;
+				//wait 500ms for other trx
+				try {
+					Thread.sleep(500);
+				} catch (InterruptedException e1) {
+				}
+			} finally {
+				if (savepoint != null) {
+					try {
+						trx.releaseSavepoint(savepoint);
+					} catch (SQLException e) {
+					}
+				}
+			}
+		}
+		//should never reach here
+		return null;
 	}	//	createSerNo
 
 }	//	MSerNoCtl

org.idempiere.test/src/org/idempiere/test/DictionaryIDs.java

@@ -553,6 +553,16 @@ public final class DictionaryIDs {
 		}
 	}
 	
+	public enum M_SerNoCtl {
+		SERIAL_NO_EXAMPLE(100);
+		
+		public final int id;
+		
+		private M_SerNoCtl(int id) {
+			this.id = id;
+		}
+	}
+	
 	public enum M_Shipper {
 		UPS(100),
 		FERTILIZER_INTERNAL_SHIPPER(50001),

org.idempiere.test/src/org/idempiere/test/model/MAttributeSetTest.java

@@ -26,13 +26,19 @@ package org.idempiere.test.model;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.util.concurrent.atomic.AtomicReference;
+
 import org.compiere.model.MAttribute;
 import org.compiere.model.MAttributeSet;
+import org.compiere.model.MAttributeSetInstance;
 import org.compiere.model.MAttributeUse;
 import org.compiere.util.Env;
+import org.compiere.util.Trx;
+import org.compiere.util.TrxRunnable;
 import org.idempiere.test.AbstractTestCase;
 import org.idempiere.test.DictionaryIDs;
 import org.junit.jupiter.api.Test;
@@ -115,4 +121,60 @@ public class MAttributeSetTest extends AbstractTestCase {
 		as.load(getTrxName());
 		assertTrue(as.isInstanceAttribute());
 	}
+	
+	@Test
+	public void testGenerateUniqueSerial() {
+		MAttributeSet mas = new MAttributeSet(Env.getCtx(), DictionaryIDs.M_AttributeSet.PATIO_CHAIR.id, null);
+		mas.setM_SerNoCtl_ID(DictionaryIDs.M_SerNoCtl.SERIAL_NO_EXAMPLE.id);
+		try {
+			mas.saveEx();
+			Trx trx1 = Trx.get(Trx.createTrxName(), true);
+			Trx trx2 = Trx.get(Trx.createTrxName(), true);
+			AtomicReference<String>atomic1 = new AtomicReference<String>(null);
+			AtomicReference<String>atomic2 = new AtomicReference<String>(null);
+			try {
+				TrxRunnable runnable1 = (trxName -> {
+					MAttributeSetInstance asi1 = new MAttributeSetInstance(Env.getCtx(), 0, mas.get_ID(), trxName);
+					String serno1 = asi1.getSerNo(true);
+					try {
+						Thread.sleep(500);
+					} catch (InterruptedException e) {
+					}
+					Trx.get(trxName, false).commit();
+					atomic1.set(serno1);					
+				});
+				TrxRunnable runnable2 = (trxName -> {
+					MAttributeSetInstance asi2 = new MAttributeSetInstance(Env.getCtx(), 0, mas.get_ID(), trx2.getTrxName());
+					String serno2 = asi2.getSerNo(true);
+					Trx.get(trxName, false).commit();
+					atomic2.set(serno2);
+				});
+				Thread t1 = new Thread(() -> {
+					Trx.run(trx1.getTrxName(), runnable1);
+				}) ;
+				Thread t2 = new Thread(() -> {
+					Trx.run(trx2.getTrxName(), runnable2);
+				});
+				t1.start();
+				t2.start();
+				try {
+					t1.join();
+				} catch (InterruptedException e) {
+				}
+				try {
+					t2.join();
+				} catch (InterruptedException e) {
+				}
+				assertNotNull(atomic1.get(), "Serial number 1 not generated");
+				assertNotNull(atomic2.get(), "Serial number 2 not generated");
+				assertNotEquals(atomic1.get(), atomic2.get(), "Duplicate serial number generated");
+			} finally {
+				trx1.close();
+				trx2.close();
+			}			
+		} finally {
+			mas.setM_SerNoCtl_ID(0);
+			mas.saveEx();
+		}
+	}
 }