Hello Sebb, I have implemented in https://issues.apache.org/bugzilla/show_bug.cgi?id=53782 what I think I have understood from your mail (at least I hope so).
Note that to be useful in your tests, we should not override teardownTest in SleepTest or JavaTest as they do nothing but logging. I didn't implement this part: >> 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. As I don't understand why we would have to do that. Please review this to be sure I understood what you meant. Regards Philippe On Mon, Aug 20, 2012 at 11:38 PM, sebb <seb...@gmail.com> wrote: > On 20 August 2012 22:02, Philippe Mouawad <philippe.moua...@gmail.com> > wrote: > > On Mon, Aug 20, 2012 at 10:41 PM, sebb <seb...@gmail.com> wrote: > > > >> 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. > >> > > > > It seems strange to me, because why make one JavaSampler handle teardown > > for all others and not each one handle it for its JavaSamplerClient > > Because testEnded is only called once per instance in the test plan, > not once per instance in each thread. > > The list of TestListeners is created by the JMeter engine before the > test plan is cloned for each thread. > > Otherwise it would be huge as well. > > >> > >> 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. > >> > > > > In my thinking, users will set > > java.samplerclient.teardown_at_thread_end=false until they migrate their > > JavaSamplerClient to new behaviour > > Why should we force users to do this? > They may have multiple clients which require different behaviour. > They may have no control over some or all of the clients (could be > bought-in). > > >> > >> 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.] > >> > > > > I don't understand what you mean. > > I'd assumed that ThreadListener was implemented by creating a list > from the test plan classes; however I see that the classes are > dynamically scanned. > So supporting ThreadListener would not require additional memory, just > additional processing time. > > >> > >> 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. > >> > > > > Sorry I am not understanding, I may be missing something, can you > explain a > > little more ? > > See above - testEnded is only invoked on the classes which were > present before the test plan was cloned. > > > > >> 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. > >> > > you mean AbstractJavaSamplerClient ? > > Oops, yes. > > >> > >> 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. > >> > > > > Sorry not clear for me. > > If we don't save references to subclasses that don't themselves > implement teardownTest, then AbstractJavaSamplerClient#teardownTest > won't be called at all. > > >> > >> 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. > > PING > > >> >> > >> >> 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. > >> > > > > > > > > -- > > Cordialement. > > Philippe Mouawad. > -- Cordialement. Philippe Mouawad.
