AMQ9844 Refactor ReloadableProperties and PropertiesLoginModuleRaceConditionTest to improve error handling and concurrency management (#1636)

Author: jeanouii

activemq-jaas/src/main/java/org/apache/activemq/jaas/ReloadableProperties.java

@@ -49,19 +49,25 @@ public synchronized Properties getProps() {
 
     public synchronized ReloadableProperties obtained() {
         if (reloadTime < 0 || (key.isReload() && hasModificationAfter(reloadTime))) {
-            props = new Properties();
+            // Load into a local variable first to preserve old data if reload fails
+            // If we assigned 'props = new Properties()' first and load() throws IOException,
+            // we'd expose empty Properties. By loading into newProps first, we preserve the
+            // old data if the reload fails.
+            final Properties newProps = new Properties();
             try {
-                load(key.file(), props);
+                load(key.file(), newProps);
+                // Only assign to the instance field after successful load
+                props = newProps;
                 invertedProps = null;
                 invertedValueProps = null;
                 regexpProps = null;
                 if (key.isDebug()) {
-                    LOG.debug("Load of: " + key);
+                    LOG.debug("Load of: {}", key);
                 }
             } catch (IOException e) {
-                LOG.error("Failed to load: " + key + ", reason:" + e.getLocalizedMessage());
+                LOG.error("Failed to load: {}, reason:{}", key, e.getLocalizedMessage());
                 if (key.isDebug()) {
-                    LOG.debug("Load of: " + key + ", failure exception" + e);
+                    LOG.debug("Load of: {}, failure exception{}", key, e);
                 }
             }
             reloadTime = System.currentTimeMillis();
@@ -119,14 +125,15 @@ public synchronized Map<String, Pattern> regexpPropertiesMap() {
     }
 
     private void load(final File source, Properties props) throws IOException {
-        FileInputStream in = new FileInputStream(source);
+        final FileInputStream in = new FileInputStream(source);
         try {
             props.load(in);
             if (key.isDecrypt()) {
                 try {
-                    EncryptionSupport.decrypt(this.props, key.getAlgorithm());
+                    // Decrypt the parameter props, not this.props (which may be the old instance)
+                    EncryptionSupport.decrypt(props, key.getAlgorithm());
                 } catch (NoClassDefFoundError e) {
-                    // this Happens whe jasypt is not on the classpath..
+                    // this Happens when jasypt is not on the classpath..
                     key.setDecrypt(false);
                     LOG.info("jasypt is not on the classpath: password decryption disabled.");
                 }

activemq-jaas/src/test/java/org/apache/activemq/jaas/PropertiesLoginModuleRaceConditionTest.java

@@ -49,6 +49,7 @@ public class PropertiesLoginModuleRaceConditionTest {
     private static final String USERS_FILE = "users.properties";
     private static final String USERNAME = "first";
     private static final String PASSWORD = "secret";
+    private static final int TOTAL_LOGIN_ATTEMPTS = 25000;
 
     @Rule
     public final ErrorCollector e = new ErrorCollector();
@@ -112,7 +113,8 @@ public void before() throws FileNotFoundException, IOException {
         options.put("org.apache.activemq.jaas.properties.group", GROUPS_FILE);
         options.put("baseDir", temp.getRoot().getAbsolutePath());
 
-        errors = new ArrayBlockingQueue<Exception>(processorCount());
+        // Large enough to hold all potential errors from concurrent attempts
+        errors = new ArrayBlockingQueue<Exception>(TOTAL_LOGIN_ATTEMPTS);
         pool = Executors.newFixedThreadPool(processorCount());
         callback = new JassCredentialCallbackHandler(USERNAME, PASSWORD);
     }
@@ -128,24 +130,27 @@ public void after() throws InterruptedException {
     public void raceConditionInUsersAndGroupsLoading() throws InterruptedException, FileNotFoundException, IOException {
 
         // Brute force approach to increase the likelihood of the race condition occurring
-        for (int i = 0; i < 25000; i++) {
-            final CountDownLatch start = new CountDownLatch(1);
-            final CountDownLatch finished = new CountDownLatch(processorCount());
-            prepareLoginThreads(start, finished);
-
-            // Releases every login thread simultaneously to increase our chances of
-            // encountering the race condition
-            start.countDown();
-
-            finished.await();
-            if (isRaceConditionDetected()) {
-                e.addError(new AssertionError("At least one race condition in PropertiesLoginModule "
-                        + "has been encountered. Please examine the "
-                        + "following stack traces for more details:"));
-                for (Exception exception : errors) {
-                    e.addError(exception);
-                }
-                return;
+        // Submit many concurrent login attempts and let the thread pool handle concurrency
+        final CountDownLatch start = new CountDownLatch(1);
+        final CountDownLatch finished = new CountDownLatch(TOTAL_LOGIN_ATTEMPTS);
+
+        // Submit all login tasks to the pool
+        for (int i = 0; i < TOTAL_LOGIN_ATTEMPTS; i++) {
+            pool.submit(new LoginTester(start, finished, errors, options, callback));
+        }
+
+        // Release all threads simultaneously to maximize concurrent load
+        start.countDown();
+
+        // Wait for all attempts to complete
+        finished.await();
+
+        if (isRaceConditionDetected()) {
+            e.addError(new AssertionError("At least one race condition in PropertiesLoginModule "
+                    + "has been encountered. Please examine the "
+                    + "following stack traces for more details:"));
+            for (Exception exception : errors) {
+                e.addError(exception);
             }
         }
     }
@@ -154,12 +159,6 @@ private boolean isRaceConditionDetected() {
         return errors.size() > 0;
     }
 
-    private void prepareLoginThreads(final CountDownLatch start, final CountDownLatch finished) {
-        for (int processor = 1; processor <= processorCount() * 2; processor++) {
-            pool.submit(new LoginTester(start, finished, errors, options, callback));
-        }
-    }
-
     private int processorCount() {
         return Runtime.getRuntime().availableProcessors();
     }