On 20 August 2012 20:36, Philippe Mouawad <philippe.moua...@gmail.com> wrote: > Then what if we implement testEnded the same way. > Looking at previous implementation I don't see why it was done this way.
It's possible that it was intentional for every instance to get notification of test ended. But whatever the reason, changing the behaviour may break clients. > In my opinion implemented as I did is the best for performances as it > cleaned up as soon as it becomes useless, I agree it breaks contract but we > could introduce a new parameter : > > - java.samplerclient.teardown_at_thread_end=true => Would clean at end > of thread Using a property to control this would potentially cause issues if there are multiple classes requiring different behaviour. It would be better to support ThreadListener, though it would increase the size of the corresponding engine list regardless of whether the client implements it. [Unless it's possible to dynamically update that list if the client implements it.] This should be treated as a separate enhancement. > - java.samplerclient.teardown_at_thread_end=false => Would clean at end > of test which would work as today but we would change impl to remove static > collection and just call releaseJavaClient() That would only call the teardownTest method once, rather than per instance. The Javadoc is not clear on this, unlike the Javadoc for testEnded, so this would potentially be a change in behaviour. I think it's a bit risky to change this now. However I think it may be possible to support existing behaviour for teardownTest whilst still reducing the memory footprint. The idea is as follows: AbstractJavaSampler implements setupTest and teardownTest, so there is no need for subclass implementations to do so. JavaSampler could check whether the actual class (or one of its superclasses, apart from AbstractJavaSampler) implements teardownTest before storing a reference. To ensure that even AbstractJavaSampler#teardownTest was called at least once, the reference could be added to the set in testEnded() before processing the set. I think that this would preserve the current behaviour as far as the client class is concerned - classes should not notice any change in behaviour. If so, it could be implemented without making the behaviour optional, though we should note the change just in case. However, it would not help for 3rd party classes that don't extend AbstractJavaSampler but directly implement JavaSamplerClient. It might also be possible to support TestListener in client classes, in which case they could use that instead of implementing teardownTest/setupTest. Implementing testEnded() would be trivial; testStarted() would require instantiating the client class earlier in the cycle. The client classname is one of the fixed strings presented in the drop-down list, so will not change once a test starts. I don't think we make any guarantees as to when the client class is initialised, so we would just need to note this in the changes. Again, maybe that should be treated as a separate enhancement. > > > > What do you think ? > Regards > Philippe > On Mon, Aug 20, 2012 at 6:13 PM, sebb <seb...@gmail.com> wrote: > >> On 20 August 2012 09:49, <pmoua...@apache.org> wrote: >> > Author: pmouawad >> > Date: Mon Aug 20 08:49:59 2012 >> > New Revision: 1374946 >> > >> > URL: http://svn.apache.org/viewvc?rev=1374946&view=rev >> > Log: >> > Bug 53743 - JavaSamplers.allSamplers static Set keeps references even >> after thread has ended >> > Bugzilla Id: 53743 >> >> -1, please revert. >> >> This does solve the memory issue, but it breaks the contract for >> >> .JavaSamplerClient#teardownTest(JavaSamplerContext) >> >> which says: >> >> "Do any clean-up required by this test at the end of a test run." >> >> teardownTest is now called at end of thread, not end of test. >> >> This may break some implementations. >> >> > Modified: >> > >> jmeter/trunk/src/protocol/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java >> > jmeter/trunk/xdocs/changes.xml >> > >> > Modified: >> jmeter/trunk/src/protocol/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java >> > URL: >> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java?rev=1374946&r1=1374945&r2=1374946&view=diff >> > >> ============================================================================== >> > --- >> jmeter/trunk/src/protocol/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java >> (original) >> > +++ >> jmeter/trunk/src/protocol/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java >> Mon Aug 20 08:49:59 2012 >> > @@ -20,7 +20,6 @@ package org.apache.jmeter.protocol.java. >> > >> > import java.util.Arrays; >> > import java.util.HashSet; >> > -import java.util.Iterator; >> > import java.util.Set; >> > >> > import org.apache.jmeter.config.Arguments; >> > @@ -31,6 +30,7 @@ import org.apache.jmeter.samplers.Entry; >> > import org.apache.jmeter.samplers.SampleResult; >> > import org.apache.jmeter.testelement.TestElement; >> > import org.apache.jmeter.testelement.TestListener; >> > +import org.apache.jmeter.testelement.ThreadListener; >> > import org.apache.jmeter.testelement.property.TestElementProperty; >> > import org.apache.jorphan.logging.LoggingManager; >> > import org.apache.log.Logger; >> > @@ -41,7 +41,7 @@ import org.apache.log.Logger; >> > * information on writing Java code to be executed by this sampler. >> > * >> > */ >> > -public class JavaSampler extends AbstractSampler implements >> TestListener { >> > +public class JavaSampler extends AbstractSampler implements >> TestListener, ThreadListener { >> > >> > private static final Logger log = >> LoggingManager.getLoggerForClass(); >> > >> > @@ -76,19 +76,10 @@ public class JavaSampler extends Abstrac >> > private transient JavaSamplerContext context = null; >> > >> > /** >> > - * Set used to register all active JavaSamplers. This is used so >> that the >> > - * samplers can be notified when the test ends. >> > - */ >> > - private static final Set<JavaSampler> allSamplers = new >> HashSet<JavaSampler>(); >> > - >> > - /** >> > * Create a JavaSampler. >> > */ >> > public JavaSampler() { >> > setArguments(new Arguments()); >> > - synchronized (allSamplers) { >> > - allSamplers.add(this); >> > - } >> > } >> > >> > /** >> > @@ -240,23 +231,9 @@ public class JavaSampler extends Abstrac >> > log.debug(whoAmI() + "\ttestStarted(" + host + ")"); >> > } >> > >> > - /** >> > - * Method called at the end of the test. This is called only on one >> instance >> > - * of JavaSampler. This method will loop through all of the other >> > - * JavaSamplers which have been registered (automatically in the >> > - * constructor) and notify them that the test has ended, allowing >> the >> > - * JavaSamplerClients to cleanup. >> > - */ >> > + /* Implements TestListener.testEnded() */ >> > public void testEnded() { >> > log.debug(whoAmI() + "\ttestEnded"); >> > - synchronized (allSamplers) { >> > - Iterator<JavaSampler> i = allSamplers.iterator(); >> > - while (i.hasNext()) { >> > - JavaSampler sampler = i.next(); >> > - sampler.releaseJavaClient(); >> > - i.remove(); >> > - } >> > - } >> > } >> > >> > /* Implements TestListener.testEnded(String) */ >> > @@ -300,4 +277,15 @@ public class JavaSampler extends Abstrac >> > String guiClass = >> configElement.getProperty(TestElement.GUI_CLASS).getStringValue(); >> > return APPLIABLE_CONFIG_CLASSES.contains(guiClass); >> > } >> > + >> > + public void threadStarted() { >> > + // NOOP >> > + } >> > + >> > + /** >> > + * Cleanup java client >> > + */ >> > + public void threadFinished() { >> > + releaseJavaClient(); >> > + } >> > } >> > >> > Modified: jmeter/trunk/xdocs/changes.xml >> > URL: >> http://svn.apache.org/viewvc/jmeter/trunk/xdocs/changes.xml?rev=1374946&r1=1374945&r2=1374946&view=diff >> > >> ============================================================================== >> > --- jmeter/trunk/xdocs/changes.xml (original) >> > +++ jmeter/trunk/xdocs/changes.xml Mon Aug 20 08:49:59 2012 >> > @@ -84,6 +84,7 @@ JSR223 Test Elements using Script file a >> > <li><bugzilla>53357</bugzilla> - JMS Point to Point reports too high >> response times in Request Response Mode</li> >> > <li><bugzilla>53440</bugzilla> - SSL connection leads to >> ArrayStoreException on JDK 6 with some KeyManagerFactory SPI</li> >> > <li><bugzilla>53511</bugzilla> - access log sampler SessionFilter >> throws NullPointerException - cookie manager not initialized properly</li> >> > +<li><bugzilla>53743</bugzilla> - JavaSamplers.allSamplers static Set >> keeps references even after thread has ended</li> >> > </ul> >> > >> > <h3>Controllers</h3> >> > >> > >> > > > > -- > Cordialement. > Philippe Mouawad.
