IDEMPIERE-6016:test case to simulate transaction timeouts and ensure no open connections remain afterwards (#2219)

* IDEMPIERE-6016:allow transaction monitor's scan timeout to a short duration

* IDEMPIERE-6016:test case to simulate transaction timeouts and ensure no open connections remain afterwards

* IDEMPIERE-6016:trx timeout make leak connection by call Connection.about but not call Connection.close(Hengsin review)

replace abort with rollback
use shorter duration for timeout test 2
add Isolatead and same thread execution class annotation

* IDEMPIERE-6016:trx timeout make leak connection by call Connection.about but not call Connection.close(Hengsin review)

---------

Co-authored-by: hengsin <hengsin@gmail.com>

org.adempiere.base/src/org/compiere/util/Trx.java

@@ -465,34 +465,25 @@ public class Trx
 	}
 	
 	/**
-	 * 	Rollback and End Transaction, Close Connection and Throws an Exception.<br/>
+	 * 	Rollback and close transaction.<br/>
 	 *  This is means to be called by the timeout monitor and developer usually shouldn't call this directly.
 	 *	@return true if success
 	 */
 	public boolean rollbackAndCloseOnTimeout() {
-		s_cache.remove(getTrxName());
+		boolean success = false;
-
-		//local
-		if (m_connection == null)
-			return true;
-
-		//	Close Connection
 		try
 		{
-			// Closes any physical connection to the database
+			rollback(true);
-			m_connection.abort(Runnable::run);
-			// return to pool manage (pool will validate and re-connect to database)
-			m_connection.close();
-			m_connection = null;
-			m_active = false;
-			fireAfterCloseEvent();
 		}
 		catch (SQLException e)
 		{
 			log.log(Level.SEVERE, m_trxName, e);
 		}
-		log.config(m_trxName);
+		finally
-		return true;
+		{
+			success = close();
+		}
+		return success;
 	}
 
 	/**
@@ -830,7 +821,7 @@ public class Trx
 	}
 	
 	/** Transaction timeout monitor class */
-	static class TrxMonitor implements Runnable
+	public static class TrxMonitor implements Runnable
 	{
 
 		public void run()

org.idempiere.test/src/org/idempiere/test/base/DBTest.java

@@ -25,8 +25,14 @@ import java.sql.SQLException;
 import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.regex.PatternSyntaxException;
 
 import org.adempiere.exceptions.DBException;
+import org.compiere.Adempiere;
 import org.compiere.model.MBPartner;
 import org.compiere.model.MColumn;
 import org.compiere.model.MOrder;
@@ -44,12 +50,17 @@ import org.idempiere.test.DictionaryIDs;
 import org.idempiere.test.LoginDetails;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.parallel.Execution;
+import org.junit.jupiter.api.parallel.ExecutionMode;
+import org.junit.jupiter.api.parallel.Isolated;
 
 /**
  * Test {@link org.compiere.util.DB} class
  * @author Teo Sarca, www.arhipac.ro
  * @author hengsin
  */
+@Isolated
+@Execution(ExecutionMode.SAME_THREAD)
 public class DBTest extends AbstractTestCase
 {
 	private static final int TEST_RECORD_ID = 103;
@@ -340,44 +351,7 @@ public class DBTest extends AbstractTestCase
 			rollback();
 		}				
 	}
-	
+
-	@Test
-	public void testTrxTimeout() 
-	{
-		try {
-			MBPartner bp = new MBPartner(Env.getCtx(), DictionaryIDs.C_BPartner.JOE_BLOCK.id, getTrxName());
-			DB.getDatabase().forUpdate(bp, 0);
-			
-			Exception exception = null;
-			Trx trx2 = Trx.get(Trx.createTrxName(), true);
-			MBPartner bp2 = new MBPartner(Env.getCtx(), DictionaryIDs.C_BPartner.JOE_BLOCK.id, trx2.getTrxName());
-			try {
-				Thread thread = new Thread (() -> { 
-					try {
-						Thread.sleep(5 * 1000);
-					} catch (InterruptedException e) {
-					}
-					if (trx2.isActive()) trx2.rollbackAndCloseOnTimeout();
-				});
-				thread.start();
-				DB.getDatabase().forUpdate(bp2, 10);
-			} catch (Exception e) {
-				exception = e;
-			} finally {
-				trx2.close();
-			}
-			assertNotNull(exception, "Exception not happens as expected");
-			assertTrue(exception instanceof DBException, "Exception not instanceof DBException");
-			DBException dbe = (DBException) exception;
-			if (DB.isPostgreSQL())
-				assertTrue("08006".equals(dbe.getSQLState()), "Trx2 not timeout as expected: " + dbe.getSQLException().getMessage());
-			else if (DB.isOracle())
-				assertTrue("08003".equals(dbe.getSQLState()), "Trx2 not timeout as expected: " + dbe.getSQLException().getMessage());
-		} finally {
-			rollback();
-		}				
-	}
-	
 	@Test
 	public void testPostgreSQLSyncColumn() {
 		if (!DB.isPostgreSQL() || !DB.getDatabase().isNativeMode())
@@ -408,6 +382,87 @@ public class DBTest extends AbstractTestCase
 			return super.newLoginDetails(testInfo);
 		}
 	}
+
+	@Test
+	public void testTrxTimeout() 
+	{
+		try {
+			MBPartner bp = new MBPartner(Env.getCtx(), DictionaryIDs.C_BPartner.JOE_BLOCK.id, getTrxName());
+			DB.getDatabase().forUpdate(bp, 0);
+			
+			Exception exception = null;
+			Trx trx2 = Trx.get(Trx.createTrxName(), true);
+			MBPartner bp2 = new MBPartner(Env.getCtx(), DictionaryIDs.C_BPartner.JOE_BLOCK.id, trx2.getTrxName());
+			try {
+				Thread thread = new Thread (() -> { 
+					try {
+						Thread.sleep(5 * 1000);
+					} catch (InterruptedException e) {
+					}
+					if (trx2.isActive()) trx2.rollbackAndCloseOnTimeout();
+				});
+				thread.start();
+				DB.getDatabase().forUpdate(bp2, 10);
+			} catch (Exception e) {
+				exception = e;
+			} finally {
+				trx2.close();
+			}
+			assertNotNull(exception, "Exception not happens as expected");
+			assertTrue(exception instanceof DBException, "Exception not instanceof DBException");
+		} finally {
+			rollback();
+		}				
+	}
+
+	public static Pattern REG_ACTIVE_CONNECT = Pattern.compile("# Busy Connections:\\s*(\\d+)\\s*,", Pattern.CASE_INSENSITIVE | Pattern.UNICODE_CASE);
 	
+	public static int getNumConnectPerStatus (String poolStatus, Pattern patternStatus) {
+		int numActiveConn = -1;
+		try {
+			
+			Matcher regexMatcher = patternStatus.matcher(poolStatus);
+			if (regexMatcher.find()) {
+				String activeConnectionStr = regexMatcher.group(1);
+				numActiveConn = Integer.parseInt(activeConnectionStr);
+			}
+		} catch (PatternSyntaxException ex) {
+			// Syntax error in the regular expression
+		}
+		return numActiveConn;
+	}
+	
+	/**
+	 * test case to simulate transaction timeouts and ensure no open connections remain afterwards
+	 */
+	@Test
+	public void testTrxTimeout2() {
+		//initial delay to give connection pool time to release active connections
+		try {
+			Thread.sleep(3000);
+		} catch (InterruptedException e) {
+		}
+		
+		//create a short duration monitor for testing
+		Trx.TrxMonitor monitor = new Trx.TrxMonitor();
+		ScheduledFuture<?> future = Adempiere.getThreadPoolExecutor().scheduleWithFixedDelay(monitor, 0, 6, TimeUnit.SECONDS);
+		int beforeActiveConnection = getNumConnectPerStatus(DB.getDatabase().getStatus(), REG_ACTIVE_CONNECT);
+		
+		Trx trx2 = Trx.get(Trx.createTrxName(), true);
+		trx2.setTimeout(3);// timeout after 3s
+		
+		DB.getSQLValueEx(trx2.getTrxName(), "SELECT 1 FROM DUAL");// to make transaction start
+		
+		try {
+			Thread.sleep(8000);//Wait for the transaction monitor to complete its task
+			
+			int afterActiveConnection = getNumConnectPerStatus(DB.getDatabase().getStatus(), REG_ACTIVE_CONNECT);
+			assertEquals(beforeActiveConnection, afterActiveConnection);
+		} catch (InterruptedException e) {
+			e.printStackTrace();
+		} finally {
+			future.cancel(true);
+		}
+	}
 	
 }