Bug 55827

Summary: Iteration over a view on a synchronized collection without obtaining a lock on the collection
Product: JMeter Reporter: Bob Atkey <bob>
Component: MainAssignee: JMeter issues mailing list <issues>
Status: NEW ---    
Severity: normal CC: p.mouawad
Priority: P2    
Version: 2.10   
Target Milestone: ---   
Hardware: PC   
OS: Linux   

Description Bob Atkey 2013-11-28 16:44:13 UTC
We ran our static analysis tool ThreadSafe [1] on version 2.10 of JMeter, which appeared to uncover a couple of concurrency issues. One of the most interesting was the possibility of an unsynchronised iteration over a synchronised collection in the class 'AbstractTestElement'.

The relevant line is 499:
   Iterator<Map.Entry<String, JMeterProperty>>  iter = propMap.entrySet().iterator();

where the propMap field always contains a synchronised collection created at line 57 in the same file.

The JDK documentation for Collections.synchronizedMap states that:

>  It is imperative that the user manually synchronize on the
> returned map when iterating over any of its collection views:
>
>  Map m = Collections.synchronizedMap(new HashMap());
>      ...
>  Set s = m.keySet();  // Needn't be in synchronized block
>      ...
>  synchronized (m) {  // Synchronizing on m, not s!
>      Iterator i = s.iterator(); // Must be in synchronized block
>      while (i.hasNext())
>          foo(i.next());
>  }
>
> Failure to follow this advice may result in non-deterministic behavior. 

It appears that the code on line 499 does not correctly synchronize on propMap, leading to the possibility of the non-deterministic behaviour the JDK documentation warns about.

We're not sure that this can actually result in a user-visible bug, but we thought you'd like to know.

We are also planning to use this finding as an example of Android-related concurrency mistakes in an article about ThreadSafe. Obviously, if you, as the developers of JMeter, have any objections to our using this as an example, then we won't.

[1] ThreadSafe is a static analysis tool for Java concurrency, developed by Contemplate Ltd.:
     http://www.contemplateltd.com/
Comment 1 Bob Atkey 2013-11-28 16:46:22 UTC
I wrote:

> We are also planning to use this finding as an example of Android-related 
> concurrency mistakes in an article about ThreadSafe.

Obviously, "Android-related" was a mistake here. I meant to write "Java-related". Sorry about that.
Comment 2 Philippe Mouawad 2013-12-06 21:27:01 UTC
Thanks for report, I was wondering why your tool does not also report line 456:
return new PropertyIteratorImpl(propMap.values());


Did you report all findings of your tool or only some examples ?
Comment 3 Bob Atkey 2013-12-11 20:12:18 UTC
> Thanks for report, I was wondering why your tool does not also report line 456:
> return new PropertyIteratorImpl(propMap.values());

Actually, our tool doesn't report the instance on line 456 because it doesn't look into the implementation of PropertyIteratorImpl while analysing the AbstractTestElement class. Therefore, it doesn't realise that everything that is passed in will have .iterator() called on it. This is certainly something that we will look at adding to ThreadSafe in the future. Thanks!

> Did you report all findings of your tool or only some examples ?

I only reported two examples of findings that I thought looked interesting. If I get more time soon I will report the rest of the interesting looking reports that ThreadSafe gives.
Comment 4 Graham Russell 2017-01-29 10:14:01 UTC
Created PR for the example identified in this issue: https://github.com/apache/jmeter/pull/260

Are there any more that the tool found (in the latest code) which should be addressed?
Comment 5 Vladimir Sitnikov 2017-02-07 22:04:45 UTC
As far as I can see, the syncrhonizedMap was added in attempt to fix https://bz.apache.org/bugzilla/show_bug.cgi?id=16304
The latest reply suggests that the synchronization did not help.

So I do not see much reason to complicate code.
I don't think it would hurt, yet I see no reason how it would help.
Comment 6 Philippe Mouawad 2017-02-07 22:29:19 UTC
(In reply to Vladimir Sitnikov from comment #5)
> As far as I can see, the syncrhonizedMap was added in attempt to fix
> https://bz.apache.org/bugzilla/show_bug.cgi?id=16304
> The latest reply suggests that the synchronization did not help.
> 
> So I do not see much reason to complicate code.
> I don't think it would hurt, yet I see no reason how it would help.

Thanks Felix for digging, interesting indeed.

A question for you, I have a doubt. In my understanding, calling synchronized in monothreaded call has a cost:
http://stackoverflow.com/questions/973518/are-synchronized-methods-slower-in-single-threaded-applications

Do you confirm this from benchmarks you did ?
Comment 7 Graham Russell 2017-02-24 22:23:41 UTC
(In reply to Vladimir Sitnikov from comment #5)
> As far as I can see, the syncrhonizedMap was added in attempt to fix
> https://bz.apache.org/bugzilla/show_bug.cgi?id=16304
> The latest reply suggests that the synchronization did not help.
> 
> So I do not see much reason to complicate code.
> I don't think it would hurt, yet I see no reason how it would help.

It does say the problem still exists in the comment yet the issue was set to resolved.

Maybe we should remove the synchronizedMap as we are accessing it in an unsafe way anyway - or just comment the code and close this bz.