JMS : Cache of InitialContext has some issues #2546

asfimport · 2011-09-17T20:16:49Z

@pmouawad (Bug 51840):
Hello,
I noticed this issue while testing JMS Subscriber and Publisher.
Methode close of InitialContextFactory is never called.
This has the following impacts:

Open JMX, run script, close it => When you reopen the old one is used
When you rerun a Test, the InitialContext is not reloaded
There is also another biggest issue:

If you use the same initialContextFactory and providerUrl but one with User /Pwd , the other one without, as these 2 infos are used as a key, this will provoke an issue for one of the samplers depending on which one is initialized first.

Regards
Philippe

Severity: normal
OS: All

asfimport · 2011-09-17T21:22:00Z

@pmouawad (migrated from Bugzilla):
(In reply to comment 0)

Hello,
I noticed this issue while testing JMS Subscriber and Publisher.
Methode close of InitialContextFactory is never called.
This has the following impacts:

Open JMX, run script, close it => When you reopen the old one is used

When you rerun a Test, the InitialContext is not reloaded
There is also another biggest issue:

If you use the same initialContextFactory and providerUrl but one with User
/Pwd , the other one without, as these 2 infos are used as a key, this will
provoke an issue for one of the samplers depending on which one is initialized
first.

Regards
Philippe

Please ignore this part:

There is also another biggest issue:

If you use the same initialContextFactory and providerUrl but one with User
/Pwd , the other one without, as these 2 infos are used as a key, this will
provoke an issue for one of the samplers depending on which one is initialized
first.

I fixed it in issue patch to issue 51691 where it has its part.
Regards
Philippe

asfimport · 2011-09-18T10:17:51Z

@pmouawad (migrated from Bugzilla):
Hello Sebb,
I implemented the TestListener in SubscriberSampler and added in PublisherSampler a call to unregister in testEnded.

By the way I also changed InitialContextFactory synchronization because it seems to me too COARSE grained, due to static synchronized.
I replaced these by a ConcurrentHashMap.

Regards
Philippe

Created attachment BUG_51840.patch: Fix to the issue

BUG_51840.patch

Index: src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/PublisherSampler.java
===================================================================
--- src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/PublisherSampler.java	(revision 1172239)
+++ src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/PublisherSampler.java	(working copy)
@@ -36,6 +36,7 @@
 import org.apache.jmeter.protocol.jms.Utils;
 import org.apache.jmeter.protocol.jms.control.gui.JMSPublisherGui;
 import org.apache.jmeter.protocol.jms.client.ClientPool;
+import org.apache.jmeter.protocol.jms.client.InitialContextFactory;
 import org.apache.jmeter.protocol.jms.client.Publisher;
 
 import org.apache.jorphan.logging.LoggingManager;
@@ -84,7 +85,7 @@
     /**
      * the implementation calls testEnded() without any parameters.
      */
-    public void testEnded(String test) {
+    public void testEnded(String host) {
         testEnded();
     }
 
@@ -96,6 +97,10 @@
     public void testEnded() {
         log.debug("PublisherSampler.testEnded called");
         ClientPool.clearClient();
+        InitialContextFactory.unregister(getJNDIInitialContextFactory(), 
+                getProviderUrl(), 
+                getUsername(),
+                getPassword());
     }
 
     public void testStarted() {
Index: src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/SubscriberSampler.java
===================================================================
--- src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/SubscriberSampler.java	(revision 1172239)
+++ src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/SubscriberSampler.java	(working copy)
@@ -25,11 +25,14 @@
 import javax.jms.TextMessage;
 import javax.naming.NamingException;
 
+import org.apache.jmeter.engine.event.LoopIterationEvent;
 import org.apache.jmeter.protocol.jms.Utils;
+import org.apache.jmeter.protocol.jms.client.InitialContextFactory;
 import org.apache.jmeter.protocol.jms.client.ReceiveSubscriber;
 import org.apache.jmeter.protocol.jms.control.gui.JMSSubscriberGui;
 import org.apache.jmeter.samplers.Interruptible;
 import org.apache.jmeter.samplers.SampleResult;
+import org.apache.jmeter.testelement.TestListener;
 import org.apache.jmeter.testelement.ThreadListener;
 import org.apache.jmeter.util.JMeterUtils;
 import org.apache.jorphan.logging.LoggingManager;
@@ -48,7 +51,7 @@
 // Note: originally the code did use the ClientPool to "share" subscribers, however since the
 // key was "this" and each sampler is unique - nothing was actually shared.
 
-public class SubscriberSampler extends BaseJMSSampler implements Interruptible, ThreadListener {
+public class SubscriberSampler extends BaseJMSSampler implements Interruptible, ThreadListener, TestListener {
 
     private static final long serialVersionUID = 240L;
 
@@ -375,4 +378,41 @@
         setProperty(STOP_BETWEEN, selected, false);                
     }
 
+    /**
+     * {@inheritDoc}
+     */
+    public void testEnded() {
+        InitialContextFactory.unregister(getJNDIInitialContextFactory(), 
+                getProviderUrl(), 
+                getUsername(),
+                getPassword());
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    public void testEnded(String host) {
+        testEnded();
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    public void testIterationStart(LoopIterationEvent event) {
+        //NOOP
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    public void testStarted() {
+        //NOOP
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    public void testStarted(String host) {
+        // NOOP        
+    }
 }
Index: src/protocol/jms/org/apache/jmeter/protocol/jms/client/InitialContextFactory.java
===================================================================
--- src/protocol/jms/org/apache/jmeter/protocol/jms/client/InitialContextFactory.java	(revision 1172239)
+++ src/protocol/jms/org/apache/jmeter/protocol/jms/client/InitialContextFactory.java	(working copy)
@@ -18,9 +18,10 @@
 
 package org.apache.jmeter.protocol.jms.client;
 
-import java.util.HashMap;
 import java.util.Iterator;
+import java.util.Map;
 import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
 
 import javax.naming.Context;
 import javax.naming.InitialContext;
@@ -36,7 +37,7 @@
 public class InitialContextFactory {
 
     //GuardedBy("this")
-    private static final HashMap<String, Context> MAP = new HashMap<String, Context>();
+    private static final ConcurrentHashMap<String, Context> MAP = new ConcurrentHashMap<String, Context>();
 
     private static final Logger log = LoggingManager.getLoggerForClass();
 
@@ -51,7 +52,7 @@
      * @return the context, never null
      * @throws NamingException 
      */
-    public static synchronized Context lookupContext(String initialContextFactory, 
+    public static Context lookupContext(String initialContextFactory, 
             String providerUrl, boolean useAuth, String securityPrincipal, String securityCredentials) throws NamingException {
         String cacheKey = createKey(initialContextFactory ,providerUrl, securityPrincipal, securityCredentials);
         Context ctx = MAP.get(cacheKey);
@@ -72,7 +73,7 @@
             } catch (Exception e) {
                 throw new NamingException(e.toString());
             }
-            MAP.put(cacheKey, ctx);
+            MAP.putIfAbsent(cacheKey, ctx);
         }
         return ctx;
     }
@@ -131,13 +132,14 @@
             return lookupContext(initialContextFactory, providerUrl, useAuth, securityPrincipal, securityCredentials);
         }
     }
+    
     /**
      * clear all the InitialContext objects.
      */
-    public synchronized static void close() { // TODO - why is this not used?
-        Iterator<?> itr = MAP.keySet().iterator();
-        while (itr.hasNext()) {
-            Context ctx = MAP.get(itr.next());
+    public static void close() { // TODO - why is this not used?
+        for (Iterator<Map.Entry<String, Context>> iterator = MAP.entrySet().iterator(); iterator.hasNext();) {
+            Map.Entry<String, Context> entry = iterator.next();
+            Context ctx = entry.getValue();
             try {
                 ctx.close();
             } catch (NamingException e) {
@@ -147,4 +149,18 @@
         MAP.clear();
         log.info("InitialContextFactory.close() called and Context instances cleaned up");
     }
-}
+
+    /**
+     * 
+     * @param initialContextFactory
+     * @param providerUrl
+     * @param securityPrincipal
+     * @param securityCredentials
+     */
+    public static void unregister(String initialContextFactory,
+            String providerUrl, String securityPrincipal,
+            String securityCredentials) {
+        String key = createKey(initialContextFactory, providerUrl, securityPrincipal, securityCredentials);
+        MAP.remove(key);
+    }
+}
\ No newline at end of file

asfimport · 2011-09-18T10:47:13Z

Sebb (migrated from Bugzilla):
Just wondering if it would not be better just to call InitialContextFactory.close() in testEnded() ?

Is there any need to unregister the contexts individually?

asfimport · 2011-09-18T12:59:25Z

@pmouawad (migrated from Bugzilla):
I prefer to make each sampler clean its own Contexts so that if some day we have an evolution that requires some Contexts not to be cleared then it will work.
The other thing that bothers me a little is that we will have listener call many times close but only the first one does something.

But I can change it if you think it's better.
Regards
Philippe

asfimport · 2011-09-18T13:44:49Z

Sebb (migrated from Bugzilla):
There cannot be any reason to retain any contexts after the end of a test.

Also, what if two samplers use the same context? If contexts are to be individually released, then there would need to be a separate key, or a usage count. That's all more complicated, and unnecessary at present.

asfimport · 2011-09-18T17:27:56Z

@pmouawad (migrated from Bugzilla):
Hello Sebb,
I changed to call close instead of unregister.
Regards
Philippen,

Created attachment BUG_51840-2.patch: Fix to issue

BUG_51840-2.patch

Index: src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/PublisherSampler.java
===================================================================
--- src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/PublisherSampler.java	(revision 1172239)
+++ src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/PublisherSampler.java	(working copy)
@@ -36,6 +36,7 @@
 import org.apache.jmeter.protocol.jms.Utils;
 import org.apache.jmeter.protocol.jms.control.gui.JMSPublisherGui;
 import org.apache.jmeter.protocol.jms.client.ClientPool;
+import org.apache.jmeter.protocol.jms.client.InitialContextFactory;
 import org.apache.jmeter.protocol.jms.client.Publisher;
 
 import org.apache.jorphan.logging.LoggingManager;
@@ -84,7 +85,7 @@
     /**
      * the implementation calls testEnded() without any parameters.
      */
-    public void testEnded(String test) {
+    public void testEnded(String host) {
         testEnded();
     }
 
@@ -96,6 +97,7 @@
     public void testEnded() {
         log.debug("PublisherSampler.testEnded called");
         ClientPool.clearClient();
+        InitialContextFactory.close();
     }
 
     public void testStarted() {
Index: src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/SubscriberSampler.java
===================================================================
--- src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/SubscriberSampler.java	(revision 1172239)
+++ src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/SubscriberSampler.java	(working copy)
@@ -25,11 +25,14 @@
 import javax.jms.TextMessage;
 import javax.naming.NamingException;
 
+import org.apache.jmeter.engine.event.LoopIterationEvent;
 import org.apache.jmeter.protocol.jms.Utils;
+import org.apache.jmeter.protocol.jms.client.InitialContextFactory;
 import org.apache.jmeter.protocol.jms.client.ReceiveSubscriber;
 import org.apache.jmeter.protocol.jms.control.gui.JMSSubscriberGui;
 import org.apache.jmeter.samplers.Interruptible;
 import org.apache.jmeter.samplers.SampleResult;
+import org.apache.jmeter.testelement.TestListener;
 import org.apache.jmeter.testelement.ThreadListener;
 import org.apache.jmeter.util.JMeterUtils;
 import org.apache.jorphan.logging.LoggingManager;
@@ -48,7 +51,7 @@
 // Note: originally the code did use the ClientPool to "share" subscribers, however since the
 // key was "this" and each sampler is unique - nothing was actually shared.
 
-public class SubscriberSampler extends BaseJMSSampler implements Interruptible, ThreadListener {
+public class SubscriberSampler extends BaseJMSSampler implements Interruptible, ThreadListener, TestListener {
 
     private static final long serialVersionUID = 240L;
 
@@ -375,4 +378,38 @@
         setProperty(STOP_BETWEEN, selected, false);                
     }
 
+    /**
+     * {@inheritDoc}
+     */
+    public void testEnded() {
+        InitialContextFactory.close();
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    public void testEnded(String host) {
+        testEnded();
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    public void testIterationStart(LoopIterationEvent event) {
+        //NOOP
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    public void testStarted() {
+        //NOOP
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    public void testStarted(String host) {
+        // NOOP        
+    }
 }
Index: src/protocol/jms/org/apache/jmeter/protocol/jms/client/InitialContextFactory.java
===================================================================
--- src/protocol/jms/org/apache/jmeter/protocol/jms/client/InitialContextFactory.java	(revision 1172239)
+++ src/protocol/jms/org/apache/jmeter/protocol/jms/client/InitialContextFactory.java	(working copy)
@@ -18,9 +18,10 @@
 
 package org.apache.jmeter.protocol.jms.client;
 
-import java.util.HashMap;
 import java.util.Iterator;
+import java.util.Map;
 import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
 
 import javax.naming.Context;
 import javax.naming.InitialContext;
@@ -36,7 +37,7 @@
 public class InitialContextFactory {
 
     //GuardedBy("this")
-    private static final HashMap<String, Context> MAP = new HashMap<String, Context>();
+    private static final ConcurrentHashMap<String, Context> MAP = new ConcurrentHashMap<String, Context>();
 
     private static final Logger log = LoggingManager.getLoggerForClass();
 
@@ -51,7 +52,7 @@
      * @return the context, never null
      * @throws NamingException 
      */
-    public static synchronized Context lookupContext(String initialContextFactory, 
+    public static Context lookupContext(String initialContextFactory, 
             String providerUrl, boolean useAuth, String securityPrincipal, String securityCredentials) throws NamingException {
         String cacheKey = createKey(initialContextFactory ,providerUrl, securityPrincipal, securityCredentials);
         Context ctx = MAP.get(cacheKey);
@@ -72,7 +73,7 @@
             } catch (Exception e) {
                 throw new NamingException(e.toString());
             }
-            MAP.put(cacheKey, ctx);
+            MAP.putIfAbsent(cacheKey, ctx);
         }
         return ctx;
     }
@@ -131,13 +132,14 @@
             return lookupContext(initialContextFactory, providerUrl, useAuth, securityPrincipal, securityCredentials);
         }
     }
+    
     /**
      * clear all the InitialContext objects.
      */
-    public synchronized static void close() { // TODO - why is this not used?
-        Iterator<?> itr = MAP.keySet().iterator();
-        while (itr.hasNext()) {
-            Context ctx = MAP.get(itr.next());
+    public static void close() { 
+        for (Iterator<Map.Entry<String, Context>> iterator = MAP.entrySet().iterator(); iterator.hasNext();) {
+            Map.Entry<String, Context> entry = iterator.next();
+            Context ctx = entry.getValue();
             try {
                 ctx.close();
             } catch (NamingException e) {
@@ -147,4 +149,4 @@
         MAP.clear();
         log.info("InitialContextFactory.close() called and Context instances cleaned up");
     }
-}
+}
\ No newline at end of file

asfimport · 2011-09-19T00:58:45Z

Sebb (migrated from Bugzilla):
Thanks for finding the bug and the patch.

I made two changes to InitialContextFactory.java:

when the context is added to the map using putIfAbsent, this might return an existing context, in which case we need to return that, rather than the one we just created, or the Map does no correspond with what we have issued.
simplified the close method - no need to iterate over the map entries when we only want the values.

URL: http://svn.apache.org/viewvc?rev=1172403&view=rev
Log:
#2546 - JMS : Cache of InitialContext has some issues

Modified:
jakarta/jmeter/trunk/src/protocol/jms/org/apache/jmeter/protocol/jms/client/InitialContextFactory.java
jakarta/jmeter/trunk/src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/PublisherSampler.java
jakarta/jmeter/trunk/src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/SubscriberSampler.java
jakarta/jmeter/trunk/xdocs/changes.xml

asfimport closed this as completed Sep 19, 2011

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JMS : Cache of InitialContext has some issues #2546

JMS : Cache of InitialContext has some issues #2546

asfimport commented Sep 17, 2011

asfimport commented Sep 17, 2011

asfimport commented Sep 18, 2011

asfimport commented Sep 18, 2011

asfimport commented Sep 18, 2011

asfimport commented Sep 18, 2011

asfimport commented Sep 18, 2011

asfimport commented Sep 19, 2011

JMS : Cache of InitialContext has some issues #2546

JMS : Cache of InitialContext has some issues #2546

Comments

asfimport commented Sep 17, 2011

asfimport commented Sep 17, 2011

asfimport commented Sep 18, 2011

asfimport commented Sep 18, 2011

asfimport commented Sep 18, 2011

asfimport commented Sep 18, 2011

asfimport commented Sep 18, 2011

asfimport commented Sep 19, 2011