jakarta-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: [Bug 51919] Random ConcurrentModificationException or NoSuchElementException in CookieManager#removeMatchingCookies when using Concurrent Download
Date Fri, 07 Oct 2011 09:36:39 GMT
On 7 October 2011 09:45, Philippe Mouawad <philippe.mouawad@gmail.com> wrote:
> Hello Sebb,
> By proxy do you mean:
> Proxy.newProxyInstance and InvocationHandler, if so aren't we missing an
> Interface (Sampler does not do the job and it does not use Entry parameter).
> If so I agree lot of job.

Yes.

> Regarding the need to clone sampler, why do we need this, is there some data
> that may be corrupted except CookieManager ?

I don't know.
We already discovered problems with CM and CacheManager, but there are
a lot of other data areas that could potentially be affected.
For example, the HC3 and HC4 clients and their connections.

As I wrote earlier, the JMeter design is based on sampler classes
being single-threaded.

Just had a quick look, and for example the HC4 currentRequest (used
for interrupting the sampler) is a potential problem.
Any instance data that can be changed by the sampling process may
cause problems.

> I implemented the approach with CookieManager in ThreadLocal and it works
> without need for thread safety.
> I will attach it to ISSUE to let you review it.

OK, I'll have a look.

However, as I just wrote, I suspect this will only fix the issue we know about.

> Regards
> Philippe
>
> On Fri, Oct 7, 2011 at 2:46 AM, sebb <sebbaz@gmail.com> wrote:
>
>> On 7 October 2011 00:00, sebb <sebbaz@gmail.com> wrote:
>> > On 6 October 2011 22:17, Philippe Mouawad <philippe.mouawad@gmail.com>
>> wrote:
>> >> Hello Sebb,
>> >> My responses below, there is one remaining point and I think we can go
>> for
>> >> implementation (i can do it if you agree).
>> >>
>> >> Regards
>> >> Philippe
>> >>
>> >> On Thu, Oct 6, 2011 at 12:01 AM, sebb <sebbaz@gmail.com> wrote:
>> >>
>> >>> On 5 October 2011 21:46, Philippe Mouawad <philippe.mouawad@gmail.com>
>> >>> wrote:
>> >>> > Hello Sebb, all,
>> >>> > First a note regarding
>> >>> > "If cookies are being ignored, then the cookie manager property
can
>> just
>> >>> be
>> >>> > cleared - i.e. there is no cookie manager."
>> >>> >
>> >>> > Just to be sure I understand, you mean "Cookies of embedded resources
>> are
>> >>> > not used", because download of embedded resources may require
>> JSESSIONID
>> >>> > cookie at least or any cookie containing Session information.
>> >>> > In this case can CookieManager be null or shouln't it be cloned
from
>> >>> parent
>> >>> > but it's result not used ? Maybe that's what you mean or I missed
>> >>> something.
>> >>>
>> >>> I forgot that cookies might be needed to access the embedded
>> >>> resources, so that won't work.
>> >>>
>> >>> > So, regarding your last comment on 51919 here is an updated
>> >>> implementation
>> >>> > proposition:
>> >>> >
>> >>> >   - Add an option "embedded_resources_use_cookies" to use Cookies
we
>> get
>> >>> >   from embedded resources (conc or serial) download,  default
value
>> will
>> >>> be
>> >>> >   true (to handle frames by default).
>> >>>
>> >>> I don't think we need the option.
>> >>>
>> >> OK
>> >>
>> >>>
>> >>> >   - Create a Bean AsynSamplerResultHolder that will hold:
>> >>> >      - HTTPSampleResult
>> >>> >      - Cookie[]
>> >>>
>> >>> Yes, something like that.
>> >>>
>> >> OK
>> >>
>> >>>
>> >>> > *Conc download:*
>> >>> >
>> >>> >   - Pass CookieManager to AsynSampler, clone it and add existing
>> cookie,
>> >>> >   and store it in ThreadLocal
>> >>>
>> >>> No need; if the cookie manager is cloned it can be stored in the normal
>> >>> place.
>> >>>
>> >>> I don't think so, because if we clone and set in normal place (call
>> >> setCookieManager() in call() method of AsyncSampler  ) as we are
>>  calling
>> >> HTTPSamplerBase.this.sample(...), we are working on same instance as
>>  Parent
>> >> Sampler (that runs conc)  it will take place of parent Cookie Manager.
>> >> Either I didn't get what you mean otherwise I think we must store it in
>> >> ThreadLocal
>> >
>> > We need to clone the sampler and the cookie manager.
>> >
>> > AsynchSampler needs to be static; it can be passed "this" when it is
>> created.
>> > Its constructor then clones this, and replaces the Cookie Manager (if
>> > any) with a copy of the CM, and copy the cookies as well.
>> >
>> > Probably need to add a copy constuctor to CM to make this easier.
>> >
>> > AsyncSampler then uses the cloned sampler to access the sample method.
>> >
>> > I'll do the first bit shortly - converting the AsychSampler to a
>> > static class - and you can do the rest.
>>
>> Unfortunately, cloning the sampler is not easy.
>> I've had a look at wrapping the sampler to intercept calls to
>> getCookieManager() but AFAICT that would require writing an
>> interceptor proxy.
>> Which is potentially a lot of work.
>>
>> I need to give this some more thought.
>>
>> >>
>> >>> >   - At end of sample:
>> >>> >      - If "embedded_resources_use_cookies" is false =>build
>> >>> > AsynSamplerResultHolder
>> >>> >      with HTTPSampleResult and empty cookies array
>> >>> >      - If "embedded_resources_use_cookies" is true =>build
>> >>> > AsynSamplerResultHolder
>> >>> >      with HTTPSampleResult and new cookies
>> >>> >   - Inside FutureResult#get loop, merge result in CookieManager
>> >>>
>> >>> Yes.
>> >>>
>> >> OK, getCookieManager.add() will do the job.
>> >>
>> >>>
>> >>> > *Serial Mode:
>> >>> > *
>> >>> >
>> >>> >   - Before start of sample, backup CookieManager
>> >>> >   - At end of sample:
>> >>> >      - If "embedded_resources_use_cookies" is false => ignore
cookies
>> >>> >      - If "embedded_resources_use_cookies" is true => add
cookies to
>> >>> backup
>> >>> >      CookieManager
>> >>>
>> >>> No need.
>> >>>
>> >> OK
>> >>
>> >>>
>> >>> >
>> >>> > Modify HTTPSamplerBase#getCookieManager to get CookieManager first
>> from
>> >>> > Thread Local then from testElement. (as today)
>> >>> >
>> >>> >
>> >>> > Waiting for your comments on this.
>> >>>
>> >>> I was thinking that ignoring cookies would simplify the problem, but
>> >>> it does not seem to.
>> >>>
>> >>> So the only change we would need to do is to clone the cookies for
>> >>> each task, and collect them again at the end.
>> >>>
>> >>> No need to make the cookie storage class thread-safe.
>> >>>
>> >>> OK
>> >>
>> >>
>> >>> > Regards
>> >>> > Philippe
>> >>> >
>> >>> > On Mon, Oct 3, 2011 at 5:02 PM, sebb <sebbaz@gmail.com> wrote:
>> >>> >
>> >>> >> On 3 October 2011 15:39, Philippe Mouawad <
>> philippe.mouawad@gmail.com>
>> >>> >> wrote:
>> >>> >> > On Mon, Oct 3, 2011 at 4:31 PM, sebb <sebbaz@gmail.com>
wrote:
>> >>> >> >
>> >>> >> >> On 3 October 2011 14:04, Philippe Mouawad <
>> >>> philippe.mouawad@gmail.com>
>> >>> >> >> wrote:
>> >>> >> >> > You are right,
>> >>> >> >> > Patch was just about quick fix before the more
impacting fix.
>> >>> >> >> >
>> >>> >> >> > Here are my propositions regarding this more
impacting fix:
>> >>> >> >> >
>> >>> >> >> >   - Add an option to make conc download use
or not cookie,
>> default
>> >>> >> value
>> >>> >> >> >   will be false.
>> >>> >> >> >   - In AsyncSampler make a Clone with current
cookies of Parent
>> >>> >> >> >   CookieManager (the one that is calling Executor)
and store it
>> in
>> >>> >> >> ThreadLocal
>> >>> >> >> >   - Change HttpSamplerBase#getCookieManager
to retrieve first
>> in
>> >>> >> Thread
>> >>> >> >> >   Local then in instance
>> >>> >> >> >   - If option is true, when reading res in
>> Future<HTTPSampleResult>
>> >>> >> loop,
>> >>> >> >> >   reinject cookies inside ParentCookie otherwise
ignore them
>> >>> >> >>
>> >>> >> >> As I wrote earlier, I'm not sure it ever makes sense
to handle
>> >>> cookies
>> >>> >> >> from embedded resources, so it would be simpler just
to ignore
>> them.
>> >>> >> >>
>> >>> >> >> I am not sure Frame couldn't be concerned by this
case, so in my
>> >>> opinion
>> >>> >> it
>> >>> >> > should be a parameter not ignored by default.
>> >>> >>
>> >>> >> I'd overlooked frame.
>> >>> >>
>> >>> >> >
>> >>> >> >> I'm looking at passing the current sampler to the
AsyncSampler
>> class,
>> >>> >> >> which can then clone it.
>> >>> >> >> I think that will create an independent set of cookies,
so there
>> can
>> >>> >> >> be no need to protect against concurrent updates.
>> >>> >> >>
>> >>> >> > Agree, that's how I imagined it, but then you agree you
have to
>> store
>> >>> >> > CookieManager in thread local ?
>> >>> >>
>> >>> >> If cookies are being ignored, then the cookie manager property
can
>> >>> >> just be cleared - i.e. there is no cookie manager.
>> >>> >>
>> >>> >> Alternatively, it will also need to be cloned.
>> >>> >>
>> >>> >> >>
>> >>> >> >> We then need to consider if non-concurrent downloads
should still
>> be
>> >>> >> >> processed for cookies.
>> >>> >> >>
>> >>> >> >>
>> >>> >> > Conc and serial should have the same behaviour
>> >>> >>
>> >>> >> >
>> >>> >> > PS : Does it mean that you are working on issue and will
fix it on
>> >>> your
>> >>> >> side
>> >>> >>
>> >>> >> Not yet decided; we need to agree on the approach first.
>> >>> >>
>> >>> >> >
>> >>> >> >>
>> >>> >> >> > Regards
>> >>> >> >> > Philippe
>> >>> >> >> >
>> >>> >> >> > On Mon, Oct 3, 2011 at 2:29 PM, sebb <sebbaz@gmail.com>
wrote:
>> >>> >> >> >
>> >>> >> >> >> On 3 October 2011 13:14, Philippe Mouawad
<
>> >>> >> philippe.mouawad@gmail.com>
>> >>> >> >> >> wrote:
>> >>> >> >> >> > Sebb,
>> >>> >> >> >> > Do you want me to provide a patch with
ConcurrentHashMap
>> where I
>> >>> >> will
>> >>> >> >> >> have
>> >>> >> >> >> > to handle null keys and values (not
same behaviour as
>> HashMap)
>> >>> or
>> >>> >> we
>> >>> >> >> >> forget
>> >>> >> >> >> > about this approach ?
>> >>> >> >> >>
>> >>> >> >> >> I don't think we have yet decided how best
to handle
>> concurrent
>> >>> >> >> downloads.
>> >>> >> >> >>
>> >>> >> >> >> e.g. should they even be setting cookies?
>> >>> >> >> >>
>> >>> >> >> >> > Regards
>> >>> >> >> >> > Philippe
>> >>> >> >> >> >
>> >>> >> >> >> > On Mon, Oct 3, 2011 at 1:58 PM, Philippe
Mouawad <
>> >>> >> >> >> philippe.mouawad@gmail.com
>> >>> >> >> >> >> wrote:
>> >>> >> >> >> >
>> >>> >> >> >> >> Hello,
>> >>> >> >> >> >> Just a little update on my test.
>> >>> >> >> >> >> I added a clear and gc before each
Map instanciation and
>> >>> results
>> >>> >> are
>> >>> >> >> >> >> different:
>> >>> >> >> >> >>
>> >>> >> >> >> >>    - HashMapput:645
>> >>> >> >> >> >>    - ConcurrentHashMapput:832
>> >>> >> >> >> >>    - ConcurrentReaderHashMapput:620
>> >>> >> >> >> >>    - HashMap get:17
>> >>> >> >> >> >>    - ConcurrentHashMap get:32
>> >>> >> >> >> >>    - ConcurrentReaderHashMap get:60
>> >>> >> >> >> >>
>> >>> >> >> >> >>
>> >>> >> >> >> >> So it kind of closes the debate,
sorry for disturbance.
>> >>> >> >> >> >> Regards
>> >>> >> >> >> >> Philippe
>> >>> >> >> >> >>
>> >>> >> >> >> >>
>> >>> >> >> >> >>
>> >>> >> >> >> >>
>> >>> >> >> >> >> public class TestMap {
>> >>> >> >> >> >>
>> >>> >> >> >> >>     public static void main(String[]
args) {
>> >>> >> >> >> >>         StopWatch timer = new
StopWatch();
>> >>> >> >> >> >>
>> >>> >> >> >> >>         Map map = new HashMap();
>> >>> >> >> >> >>         testPut(timer, map,
"HashMap");
>> >>> >> >> >> >>         timer.reset();
>> >>> >> >> >> >>
>> >>> >> >> >> >>         map.clear();
>> >>> >> >> >> >>         System.gc();
>> >>> >> >> >> >>
>> >>> >> >> >> >>         map = new ConcurrentHashMap();
>> >>> >> >> >> >>         testPut(timer, map,
"ConcurrentHashMap");
>> >>> >> >> >> >>         timer.reset();
>> >>> >> >> >> >>
>> >>> >> >> >> >>         map.clear();
>> >>> >> >> >> >>         System.gc();
>> >>> >> >> >> >>
>> >>> >> >> >> >>         map = new ConcurrentReaderHashMap();
>> >>> >> >> >> >>         testPut(timer, map,
"ConcurrentReaderHashMap");
>> >>> >> >> >> >>         timer.reset();
>> >>> >> >> >> >>
>> >>> >> >> >> >>
>> >>> >> >> >> >>         map.clear();
>> >>> >> >> >> >>         System.gc();
>> >>> >> >> >> >>
>> >>> >> >> >> >>         map = new HashMap();
>> >>> >> >> >> >>         testGet(timer, map,
"HashMap");
>> >>> >> >> >> >>         timer.reset();
>> >>> >> >> >> >>
>> >>> >> >> >> >>         map.clear();
>> >>> >> >> >> >>         System.gc();
>> >>> >> >> >> >>
>> >>> >> >> >> >>         map = new ConcurrentHashMap();
>> >>> >> >> >> >>         testGet(timer, map,
"ConcurrentHashMap");
>> >>> >> >> >> >>         timer.reset();
>> >>> >> >> >> >>
>> >>> >> >> >> >>         map.clear();
>> >>> >> >> >> >>         System.gc();
>> >>> >> >> >> >>
>> >>> >> >> >> >>         map = new ConcurrentReaderHashMap();
>> >>> >> >> >> >>         testGet(timer, map,
"ConcurrentReaderHashMap");
>> >>> >> >> >> >>         timer.reset();
>> >>> >> >> >> >>     }
>> >>> >> >> >> >>
>> >>> >> >> >> >>     /**
>> >>> >> >> >> >>      * @param timer
>> >>> >> >> >> >>      */
>> >>> >> >> >> >>     private static void testGet(StopWatch
timer, Map map,
>> >>> String
>> >>> >> >> type) {
>> >>> >> >> >> >>         timer.start();
>> >>> >> >> >> >>         for (int i = 0; i <
1000000; i++) {
>> >>> >> >> >> >>             map.get(i);
>> >>> >> >> >> >>         }
>> >>> >> >> >> >>         timer.stop();
>> >>> >> >> >> >>         System.out.println(type+"
get:"+timer.getTime());
>> >>> >> >> >> >>     }
>> >>> >> >> >> >>
>> >>> >> >> >> >>     /**
>> >>> >> >> >> >>      * @param timer
>> >>> >> >> >> >>      */
>> >>> >> >> >> >>     private static void testPut(StopWatch
timer, Map map,
>> >>> String
>> >>> >> >> type) {
>> >>> >> >> >> >>         timer.start();
>> >>> >> >> >> >>         for (int i = 0; i <
1000000; i++) {
>> >>> >> >> >> >>             map.put(i,i);
>> >>> >> >> >> >>         }
>> >>> >> >> >> >>         timer.stop();
>> >>> >> >> >> >>         System.out.println(type+"put:"+timer.getTime());
>> >>> >> >> >> >>     }
>> >>> >> >> >> >>
>> >>> >> >> >> >>
>> >>> >> >> >> >> Regards
>> >>> >> >> >> >> Philippe
>> >>> >> >> >> >>
>> >>> >> >> >> >>
>> >>> >> >> >> >> On Mon, Oct 3, 2011 at 1:37 PM,
sebb <sebbaz@gmail.com>
>> wrote:
>> >>> >> >> >> >>
>> >>> >> >> >> >>> On 3 October 2011 12:15, Rainer
Jung <
>> rainer.jung@kippdata.de
>> >>> >
>> >>> >> >> wrote:
>> >>> >> >> >> >>> > On 02.10.2011 23:17, Philippe
Mouawad wrote:
>> >>> >> >> >> >>> >> Ok, hope we can do
the same.
>> >>> >> >> >> >>> >
>> >>> >> >> >> >>> > See https://issues.apache.org/jira/browse/XMLBEANS-447
>> >>> >> >> >> >>> >
>> >>> >> >> >> >>> > We are not the only people,
who doubt it's correct to
>> >>> include
>> >>> >> that
>> >>> >> >> >> class
>> >>> >> >> >> >>> ...
>> >>> >> >> >> >>> >
>> >>> >> >> >> >>> > There was also a discussion
some time ago in another ASF
>> >>> >> project,
>> >>> >> >> >> >>> > because the Sun license
which is cited by Doug Lea has a
>> "no
>> >>> >> use
>> >>> >> >> in
>> >>> >> >> >> >>> > nuclear facilities clause",
which make it non-usable as
>> an
>> >>> Open
>> >>> >> >> >> Source
>> >>> >> >> >> >>> > license.
>> >>> >> >> >> >>> >
>> >>> >> >> >> >>> > It looks like we are stuck
here.
>> >>> >> >> >> >>>
>> >>> >> >> >> >>> Yes, apart from the binary option
which brings in lots of
>> >>> >> unneeded
>> >>> >> >> >> code.
>> >>> >> >> >> >>>
>> >>> >> >> >> >>> > And the mentioning of the
Harmony class in the above
>> cited
>> >>> >> issue
>> >>> >> >> is a
>> >>> >> >> >> >>> > red herring. Diffing it
with the JDK 5 standard
>> >>> >> ConcurrentHashMap
>> >>> >> >> >> shows
>> >>> >> >> >> >>> > little difference, so I
doubt it will be much faster
>> (though
>> >>> I
>> >>> >> >> didn't
>> >>> >> >> >> >>> > inspect the delta in detail).
>> >>> >> >> >> >>>
>> >>> >> >> >> >>> I think the Harmony class was
only mentioned as a means of
>> >>> >> >> supporting
>> >>> >> >> >> >>> Java 1.4, not as an alternative
faster implementation.
>> >>> >> >> >> >>>
>> >>> >> >> >> >>> >> I must say I don't
understand why
>> ConcurrentReaderHashMap
>> >>> is
>> >>> >> not
>> >>> >> >> in
>> >>> >> >> >> >>> JDK.
>> >>> >> >> >> >>> >
>> >>> >> >> >> >>> > There's a discussion list
for JSR166 (the concurrency
>> stuff
>> >>> >> lead
>> >>> >> >> by
>> >>> >> >> >> Doug
>> >>> >> >> >> >>> > Lea) mentioned on the JSR
166 page maintained by Doug
>> Lea.
>> >>> So
>> >>> >> >> maybe
>> >>> >> >> >> you
>> >>> >> >> >> >>> > can post a technical question
there (what's the right
>> class
>> >>> >> that
>> >>> >> >> >> solve
>> >>> >> >> >> >>> > the following problem ...
and doesn't have sun licensing
>> >>> >> >> >> restrictions).
>> >>> >> >> >> >>> >
>> >>> >> >> >> >>> > Regards,
>> >>> >> >> >> >>> >
>> >>> >> >> >> >>> > Rainer
>> >>> >> >> >> >>> >
>> >>> >> >> >> >>> >
>> >>> >> >> >> >>> >
>> >>> >> >>
>> ---------------------------------------------------------------------
>> >>> >> >> >> >>> > To unsubscribe, e-mail:
>> dev-unsubscribe@jakarta.apache.org
>> >>> >> >> >> >>> > For additional commands,
e-mail:
>> >>> dev-help@jakarta.apache.org
>> >>> >> >> >> >>> >
>> >>> >> >> >> >>> >
>> >>> >> >> >> >>>
>> >>> >> >> >> >>>
>> >>> >> >>
>> ---------------------------------------------------------------------
>> >>> >> >> >> >>> To unsubscribe, e-mail:
>> dev-unsubscribe@jakarta.apache.org
>> >>> >> >> >> >>> For additional commands, e-mail:
>> dev-help@jakarta.apache.org
>> >>> >> >> >> >>>
>> >>> >> >> >> >>>
>> >>> >> >> >> >>
>> >>> >> >> >> >>
>> >>> >> >> >> >> --
>> >>> >> >> >> >> Cordialement.
>> >>> >> >> >> >> Philippe Mouawad.
>> >>> >> >> >> >>
>> >>> >> >> >> >>
>> >>> >> >> >> >>
>> >>> >> >> >> >>
>> >>> >> >> >> >
>> >>> >> >> >> >
>> >>> >> >> >> > --
>> >>> >> >> >> > Cordialement.
>> >>> >> >> >> > Philippe Mouawad.
>> >>> >> >> >> >
>> >>> >> >> >>
>> >>> >> >> >>
>> >>> ---------------------------------------------------------------------
>> >>> >> >> >> To unsubscribe, e-mail: dev-unsubscribe@jakarta.apache.org
>> >>> >> >> >> For additional commands, e-mail: dev-help@jakarta.apache.org
>> >>> >> >> >>
>> >>> >> >> >>
>> >>> >> >> >
>> >>> >> >> >
>> >>> >> >> > --
>> >>> >> >> > Cordialement.
>> >>> >> >> > Philippe Mouawad.
>> >>> >> >> >
>> >>> >> >>
>> >>> >> >>
>> ---------------------------------------------------------------------
>> >>> >> >> To unsubscribe, e-mail: dev-unsubscribe@jakarta.apache.org
>> >>> >> >> For additional commands, e-mail: dev-help@jakarta.apache.org
>> >>> >> >>
>> >>> >> >>
>> >>> >> >
>> >>> >> >
>> >>> >> > --
>> >>> >> > Cordialement.
>> >>> >> > Philippe Mouawad.
>> >>> >> >
>> >>> >>
>> >>> >>
>> ---------------------------------------------------------------------
>> >>> >> To unsubscribe, e-mail: dev-unsubscribe@jakarta.apache.org
>> >>> >> For additional commands, e-mail: dev-help@jakarta.apache.org
>> >>> >>
>> >>> >>
>> >>> >
>> >>> >
>> >>> > --
>> >>> > Cordialement.
>> >>> > Philippe Mouawad.
>> >>> >
>> >>>
>> >>> ---------------------------------------------------------------------
>> >>> To unsubscribe, e-mail: dev-unsubscribe@jakarta.apache.org
>> >>> For additional commands, e-mail: dev-help@jakarta.apache.org
>> >>>
>> >>>
>> >>
>> >>
>> >> --
>> >> Cordialement.
>> >> Philippe Mouawad.
>> >>
>> >
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@jakarta.apache.org
>> For additional commands, e-mail: dev-help@jakarta.apache.org
>>
>>
>
>
> --
> Cordialement.
> Philippe Mouawad.
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: dev-help@jakarta.apache.org


Mime
View raw message