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 > > 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 > > 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. > > 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 ? > 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 ? > > 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. > > 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. > -- Cordialement. Philippe Mouawad.
