Bug 51919 - Random ConcurrentModificationException or NoSuchElementException in CookieManager#removeMatchingCookies when using Concurrent Download
Random ConcurrentModificationException or NoSuchElementException in CookieMan...
Status: RESOLVED FIXED
Product: JMeter
Classification: Unclassified
Component: HTTP
2.5.1
All All
: P2 major (vote)
: ---
Assigned To: JMeter issues mailing list
:
Depends on: 51918
Blocks:
  Show dependency tree
 
Reported: 2011-09-29 20:33 UTC by Philippe Mouawad
Modified: 2011-10-23 20:59 UTC (History)
1 user (show)



Attachments
Scenario that reproduces the issue (40.82 KB, application/octet-stream)
2011-09-29 20:33 UTC, Philippe Mouawad
Details
Fix to the issue (1.02 KB, patch)
2011-09-30 07:43 UTC, Philippe Mouawad
Details | Diff
Fix to issue (6.16 KB, patch)
2011-10-01 09:51 UTC, Philippe Mouawad
Details | Diff
Example of ConcurrentModificationException with Collections.synchronizedCollection (1.30 KB, text/plain)
2011-10-01 12:51 UTC, Philippe Mouawad
Details
Fix to issue (9.36 KB, patch)
2011-10-01 14:09 UTC, Philippe Mouawad
Details | Diff
Proposition with CookieManager in ThreadLocal (8.31 KB, patch)
2011-10-07 08:46 UTC, Philippe Mouawad
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Mouawad 2011-09-29 20:33:43 UTC
Created attachment 27636 [details]
Scenario that reproduces the issue

Hello,
Using options:
- HTTPCLient 3.1
- Retrieve All Embedded Resources from HTML
- Use Concurrent Pool to 5

I randomly get those 2 exceptions:
2011/09/29 22:31:16 WARN  - jmeter.protocol.http.sampler.HTTPSamplerBase: Execution issue when fetching embedded resources java.util.concurrent.ExecutionException: java.util.ConcurrentModificationException
	at java.util.concurrent.FutureTask$Sync.innerGet(FutureTask.java:222)
	at java.util.concurrent.FutureTask.get(FutureTask.java:83)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase.downloadPageResources(HTTPSamplerBase.java:1213)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase.resultProcessing(HTTPSamplerBase.java:1428)
	at org.apache.jmeter.protocol.http.sampler.HTTPAbstractImpl.resultProcessing(HTTPAbstractImpl.java:244)
	at org.apache.jmeter.protocol.http.sampler.HTTPHC3Impl.sample(HTTPHC3Impl.java:327)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerProxy.sample(HTTPSamplerProxy.java:62)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase.sample(HTTPSamplerBase.java:1019)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase.sample(HTTPSamplerBase.java:1005)
	at org.apache.jmeter.threads.JMeterThread.process_sampler(JMeterThread.java:411)
	at org.apache.jmeter.threads.JMeterThread.run(JMeterThread.java:297)
	at java.lang.Thread.run(Thread.java:680)
Caused by: java.util.ConcurrentModificationException
	at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:372)
	at java.util.AbstractList$Itr.next(AbstractList.java:343)
	at org.apache.jmeter.testelement.property.PropertyIteratorImpl.next(PropertyIteratorImpl.java:39)
	at org.apache.jmeter.protocol.http.control.CookieManager.removeMatchingCookies(CookieManager.java:466)
	at org.apache.jmeter.protocol.http.control.CookieManager.add(CookieManager.java:258)
	at org.apache.jmeter.protocol.http.control.CookieManager.addCookieFromHeader(CookieManager.java:430)
	at org.apache.jmeter.protocol.http.sampler.HTTPHC3Impl.saveConnectionCookies(HTTPHC3Impl.java:1071)
	at org.apache.jmeter.protocol.http.sampler.HTTPHC3Impl.sample(HTTPHC3Impl.java:319)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerProxy.sample(HTTPSamplerProxy.java:62)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase$ASyncSample.call(HTTPSamplerBase.java:1709)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase$ASyncSample.call(HTTPSamplerBase.java:1)
	at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
	at java.util.concurrent.FutureTask.run(FutureTask.java:138)
	at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
	... 1 more

Or 2011/09/29 21:59:12 WARN  - jmeter.protocol.http.sampler.HTTPSamplerBase: Execution issue when fetching embedded resources java.util.concurrent.ExecutionException: java.util.NoSuchElementException
	at java.util.concurrent.FutureTask$Sync.innerGet(FutureTask.java:222)
	at java.util.concurrent.FutureTask.get(FutureTask.java:83)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase.downloadPageResources(HTTPSamplerBase.java:1213)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase.resultProcessing(HTTPSamplerBase.java:1428)
	at org.apache.jmeter.protocol.http.sampler.HTTPAbstractImpl.resultProcessing(HTTPAbstractImpl.java:244)
	at org.apache.jmeter.protocol.http.sampler.HTTPHC3Impl.sample(HTTPHC3Impl.java:327)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerProxy.sample(HTTPSamplerProxy.java:62)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase.sample(HTTPSamplerBase.java:1019)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase.sample(HTTPSamplerBase.java:1005)
	at org.apache.jmeter.threads.JMeterThread.process_sampler(JMeterThread.java:411)
	at org.apache.jmeter.threads.JMeterThread.run(JMeterThread.java:297)
	at java.lang.Thread.run(Thread.java:680)
Caused by: java.util.NoSuchElementException
	at java.util.AbstractList$Itr.next(AbstractList.java:350)
	at org.apache.jmeter.testelement.property.PropertyIteratorImpl.next(PropertyIteratorImpl.java:39)
	at org.apache.jmeter.protocol.http.control.CookieManager.removeMatchingCookies(CookieManager.java:466)
	at org.apache.jmeter.protocol.http.control.CookieManager.add(CookieManager.java:258)
	at org.apache.jmeter.protocol.http.control.CookieManager.addCookieFromHeader(CookieManager.java:430)
	at org.apache.jmeter.protocol.http.sampler.HTTPHC3Impl.saveConnectionCookies(HTTPHC3Impl.java:1071)
	at org.apache.jmeter.protocol.http.sampler.HTTPHC3Impl.sample(HTTPHC3Impl.java:319)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerProxy.sample(HTTPSamplerProxy.java:62)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase$ASyncSample.call(HTTPSamplerBase.java:1709)
	at org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase$ASyncSample.call(HTTPSamplerBase.java:1)
	at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
	at java.util.concurrent.FutureTask.run(FutureTask.java:138)
	at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
	... 1 more


To reproduce issue set number of threads to > 20 you will have more chance to get it.
Regards
Philippe Mouawad
Comment 1 Philippe Mouawad 2011-09-30 07:43:39 UTC
Created attachment 27643 [details]
Fix to the issue

Hello,
I synchronized 2 methods of CookieManager that were concerned.
Since CookieManager is per User thread, there should be performance impact only when concurrent download of embedded resources is on.
I think it is acceptable, and browser have the same isse I think.
I first thought about cloning CookieManager but I don't think it's right.

Regards
Philippe
Comment 2 Milamber 2011-10-01 07:34:04 UTC
A better way is to synchronize only the code section referred to the variables from JMeterContext
(in add() and removeMatchingCookies() methods, I thinks)
Comment 3 Philippe Mouawad 2011-10-01 09:51:48 UTC
Created attachment 27654 [details]
Fix to issue

Hello Milamber,
I changed my initial patch to use ReentrantLock.
Regards
Philippe
Comment 4 Sebb 2011-10-01 10:28:29 UTC
Cookies are currently stored in an ArrayList. The Javadoc says:

"If multiple threads access an ArrayList instance concurrently, and at least one of the threads modifies the list structurally, it must be synchronized externally."

This means that all accesses must be synchronised, not just modifications.

The simplest way to do this would be to use Collections.synchronizedList.

There is a concurrent list:

http://download.oracle.com/javase/6/docs/api/java/util/concurrent/CopyOnWriteArrayList.html

However this is only worth it if there are many more reads than writes.
I'm not sure that's the case with Cookies.

==

An alternative might be to somehow defer the cookie setting to the main sample thread. It should not matter if the cookies are stored after all the resources have been read, because the order of responses is not guaranteed. Any cookies required by the resource download must exist before the request is made.
Comment 5 Sebb 2011-10-01 10:35:46 UTC
(In reply to comment #3)
> Created attachment 27654 [details]
> Fix to issue
> 
> Hello Milamber,
> I changed my initial patch to use ReentrantLock.
> Regards
> Philippe

The CONTEXT_VARIABLES_LOCK needs to be applied to reads as well as writes.

A write lock only protects against lost updates, it does not guarantee thread-safety.
Comment 6 Philippe Mouawad 2011-10-01 11:04:31 UTC
What if I remove CONTEXT_VARIABLES_LOCK and use a ConcurrentHashMap in JMeterVariables?

Regards
Philippe
Comment 7 Sebb 2011-10-01 11:14:16 UTC
(In reply to comment #6)
> What if I remove CONTEXT_VARIABLES_LOCK and use a ConcurrentHashMap in
> JMeterVariables?

That would solve the immediate problem for JMeterVariables.
It probably would not have too much of a performance or memory impact.

However, that is only one aspect of this issue; JMeter was designed on the basis that samplers etc are cloned for each thread and so don't need to worry about thread-safety issues.
Comment 8 Philippe Mouawad 2011-10-01 11:30:39 UTC
(In reply to comment #4)
> Cookies are currently stored in an ArrayList. The Javadoc says:
> 
> "If multiple threads access an ArrayList instance concurrently, and at least
> one of the threads modifies the list structurally, it must be synchronized
> externally."
> 
> This means that all accesses must be synchronised, not just modifications.
> 
> The simplest way to do this would be to use Collections.synchronizedList.
> 
> There is a concurrent list:
> 
> http://download.oracle.com/javase/6/docs/api/java/util/concurrent/CopyOnWriteArrayList.html
> 
> However this is only worth it if there are many more reads than writes.
> I'm not sure that's the case with Cookies.
> 
> ==
> 
> An alternative might be to somehow defer the cookie setting to the main sample
> thread. It should not matter if the cookies are stored after all the resources
> have been read, because the order of responses is not guaranteed. Any cookies
> required by the resource download must exist before the request is made.

Hello Sebb,
I knwo about this rule regarding the need to synchronize all methods.
But sometimes it is acceptable to only synchronize methods that modify, the
trade-off will be that:
- You can get obsolete informations (size, element removed)
- But collection state will stay OK.
That's the approach I chose.

I used it a lot of times and I am sure it works on ArrayList. It can have issues with LinkerList but not arrayList.

I think Collections.synchronizedList() can have performance impact, I am not
sure I understand what you want to do, you want to modify CollectionProperty to
use a Collections.synchronizedList() wrapper ?

Regards
Philippe
Comment 9 Sebb 2011-10-01 11:44:23 UTC
(In reply to comment #8)
> (In reply to comment #4)
> > Cookies are currently stored in an ArrayList. The Javadoc says:
> > 
> > "If multiple threads access an ArrayList instance concurrently, and at least
> > one of the threads modifies the list structurally, it must be synchronized
> > externally."
> > 
> > This means that all accesses must be synchronised, not just modifications.
> > 
> > The simplest way to do this would be to use Collections.synchronizedList.
> > 
> > There is a concurrent list:
> > 
> > http://download.oracle.com/javase/6/docs/api/java/util/concurrent/CopyOnWriteArrayList.html
> > 
> > However this is only worth it if there are many more reads than writes.
> > I'm not sure that's the case with Cookies.
> > 
> > ==
> > 
> > An alternative might be to somehow defer the cookie setting to the main sample
> > thread. It should not matter if the cookies are stored after all the resources
> > have been read, because the order of responses is not guaranteed. Any cookies
> > required by the resource download must exist before the request is made.
> 
> Hello Sebb,
> I knwo about this rule regarding the need to synchronize all methods.
> But sometimes it is acceptable to only synchronize methods that modify, the
> trade-off will be that:
> - You can get obsolete informations (size, element removed)
> - But collection state will stay OK.

A thread that reads the collection can still experience ConcurrentModificationException if it does not use synch.

> That's the approach I chose.
> 
> I used it a lot of times and I am sure it works on ArrayList. It can have
> issues with LinkerList but not arrayList.

I think you have been lucky.

> I think Collections.synchronizedList() can have performance impact, I am not
> sure I understand what you want to do, you want to modify CollectionProperty to
> use a Collections.synchronizedList() wrapper ?

I know it has an impact; that's why we should not rush into partial fixes.

But I intended it only for the Cookie ArrayList.
Comment 10 Philippe Mouawad 2011-10-01 12:51:27 UTC
Created attachment 27656 [details]
Example of ConcurrentModificationException with Collections.synchronizedCollection

Hello Sebb,
Just to clarify what I think.
In my opinion wrapping the List with Collections.synchronizedList doesn't protect from ConcurrentModificationException when iterator are used, see attached example.

So Locking is necessary when Iterations are done.

This is just to say that I think my fix with ReentrantLock + ConcurrentHashMap in JMEterVariables should fix the issue in immediate although I agree a better long term solution is to be found.

At present, using concurrent download in a test is broken.

PS: And trust me I was not lucky regarding what I did on syncing in the past :-). Fixes concerned high concurrent systems like ecommerce websites.
Regards
Philippe
Comment 11 Philippe Mouawad 2011-10-01 14:09:24 UTC
Created attachment 27657 [details]
Fix to issue

Fix that uses ConcurrentReaderHashmap.
Regards
Philippe Mouawad
Comment 12 Sebb 2011-10-04 23:54:18 UTC
Another way to solve this would be to process any cookies on return from the asynch. task.

This would mean changing the task to return a different class that included the cookies. If the cookie array list were cloned for each thread, any differences could be merged back on return.
Comment 13 Philippe Mouawad 2011-10-07 08:46:36 UTC
Created attachment 27716 [details]
Proposition with CookieManager in ThreadLocal

A discussed on dev list, the prototype.
Comment 14 Philippe Mouawad 2011-10-23 17:37:26 UTC
Date: Sun Oct 23 17:38:38 2011
New Revision: 1187939

URL: http://svn.apache.org/viewvc?rev=1187939&view=rev
Log:
Bug 51919 - Random ConcurrentModificationException or NoSuchElementException in CookieManager#removeMatchingCookies when using Concurrent Download

Fix proposition:
- Clone Sampler
- Clone CookieManager for each AsyncSampler
- Implement custom thread factory to add thread end notification that will call threadFinished on cloned sampler

 we can revert if you don't agree

Modified:
   jakarta/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
   jakarta/jmeter/trunk/xdocs/changes.xml