On Thursday, October 16, 2014, Vladimir Sitnikov < [email protected]> wrote:
> > know wether CompoundVariables > >which are in AbstractFunction subclasses values You didn't read correctly:) I think the main issue in this topic is to know wether CompoundVariables which are in AbstractFunction subclasses values are thread safe (to the exception of the non thread safe functions). I am saying values field is array of compoundvariables , values field is in AbstractFunction subclasses. Of course CompoundVariables do not extend AbstractFunction ! > > I am looking at commit e9228ccf8cca9b130c8730771693cbda024f8bb0 > git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1632410 and > it reads that CompoundVariable does not extend AbstractFunction. > > >Today with synchronized we ensure that: > >- While execute is being called by one thread, setParameters and other > >execute() (from other threads) are blocked. > > I hope we can forbid setParameters usage after JMeterThread start. > I understand bad things can happen if setParameters and execute are > performed concurrently. > I have very little idea how JMeter works internally, however I hope it does > not do such things. > > >And due to the fact that it seems we never enter this condition of > >CompoundVariable#execute() which alters an instance variable : > > 1) If the CV in question is dynamic (in terms of isDynamic==true after each > execution), then execute is thread safe modulo thread safety of embedded > functions/variables. > > 2) If the CV in question is not dynamic (e.g. it does not contain > Functions/SimpleVariable), then CV is NOT thread-safe: isDynamic=false & > permanentResults =... are not ordered, thus if multiple threads execute > CV.execute concurrently, then a thread can observe isDynamic==false and > stale value in permanentResults (== it would return wrong results) This is what needs to be checked, but it requires reading jmeter code, I can understand it can take time :) > > I see the following options to fix that (in order of my preference): > 2.1) move "isDynamic" initialization to setParameters. It looks like it is > just a instanceof check. If we can't calculate initial value in > setParameters method (i.e. we must compute the value at the first execution > of execute method), then initial value of permanentResults can be set to > null and the check in execute() should read "if (isDynamic || > permanentResults==null)". > > 2.2) You can make isDynamic volatile and write code as > "permanentResults=results.toString(); > isDynamic=false" (in this order!). This is a minimal change. > > > PS. I would replace LinkedList compiledElements with ArrayList: it is > faster, more memory efficient, etc and I would use indexed iteration in > execute (to avoid creation of Iterator). It does not hurt readability in > that particular method from my point of view. > > > Vladimir > -- Cordialement. Philippe Mouawad.
