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 00:46:45 GMT
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


Mime
View raw message