Hello, By the way I think there is also an issue in CacheManager due to the use of InheritableThreadLocal (which is not thread safe). Indeed this Map is shared by concurrent children thread, so Map is accessed concurrently but not thread-safe => ISSUE I made a little main sample that I can attach to an issue if you want to see Map is shared
See this: - http://www.ibm.com/developerworks/java/library/j-threads3/index.html Regards Philippe Mouawad On Mon, Oct 3, 2011 at 5:28 PM, sebb <seb...@gmail.com> wrote: > On 3 October 2011 16:07, Shmuel Krakower <shmul...@gmail.com> wrote: > > Hi, > > Why isn't Cookie manager implemented the way Cache manager is (using a > > thread local hash map)? > > Probably mainly historical. > > Cookies are currently stored in an ArrayList, but one could perhaps > use a HashMap instead. > Though that won't work if cookie names can change - the current > implementation does allow pre-defined cookies to be defined in terms > of variables/functions. > > Is there a use-case for variable cookie names? Perhaps; it seems quite > likely that variable values would be useful. > > I don't know off-hand if that would still work if the code switched to > (Inheritable)ThreadLocal. > > > If it would be implemented the same way - the fix should be the same as > on > > the Cache manager case ( > > https://issues.apache.org/bugzilla/show_bug.cgi?id=51752) > > > > Regards, > > Shmuel Krakower. > > > > > > On Mon, Oct 3, 2011 at 5:02 PM, sebb <seb...@gmail.com> wrote: > > > >> On 3 October 2011 15:39, Philippe Mouawad <philippe.moua...@gmail.com> > >> wrote: > >> > On Mon, Oct 3, 2011 at 4:31 PM, sebb <seb...@gmail.com> wrote: > >> > > >> >> On 3 October 2011 14:04, Philippe Mouawad < > philippe.moua...@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 <seb...@gmail.com> wrote: > >> >> > > >> >> >> On 3 October 2011 13:14, Philippe Mouawad < > >> philippe.moua...@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.moua...@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 <seb...@gmail.com> wrote: > >> >> >> >> > >> >> >> >>> On 3 October 2011 12:15, Rainer Jung <rainer.j...@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-unsubscr...@jakarta.apache.org > >> >> >> >>> > For additional commands, e-mail: > dev-h...@jakarta.apache.org > >> >> >> >>> > > >> >> >> >>> > > >> >> >> >>> > >> >> >> >>> > >> >> --------------------------------------------------------------------- > >> >> >> >>> To unsubscribe, e-mail: dev-unsubscr...@jakarta.apache.org > >> >> >> >>> For additional commands, e-mail: dev-h...@jakarta.apache.org > >> >> >> >>> > >> >> >> >>> > >> >> >> >> > >> >> >> >> > >> >> >> >> -- > >> >> >> >> Cordialement. > >> >> >> >> Philippe Mouawad. > >> >> >> >> > >> >> >> >> > >> >> >> >> > >> >> >> >> > >> >> >> > > >> >> >> > > >> >> >> > -- > >> >> >> > Cordialement. > >> >> >> > Philippe Mouawad. > >> >> >> > > >> >> >> > >> >> >> > --------------------------------------------------------------------- > >> >> >> To unsubscribe, e-mail: dev-unsubscr...@jakarta.apache.org > >> >> >> For additional commands, e-mail: dev-h...@jakarta.apache.org > >> >> >> > >> >> >> > >> >> > > >> >> > > >> >> > -- > >> >> > Cordialement. > >> >> > Philippe Mouawad. > >> >> > > >> >> > >> >> --------------------------------------------------------------------- > >> >> To unsubscribe, e-mail: dev-unsubscr...@jakarta.apache.org > >> >> For additional commands, e-mail: dev-h...@jakarta.apache.org > >> >> > >> >> > >> > > >> > > >> > -- > >> > Cordialement. > >> > Philippe Mouawad. > >> > > >> > >> --------------------------------------------------------------------- > >> To unsubscribe, e-mail: dev-unsubscr...@jakarta.apache.org > >> For additional commands, e-mail: dev-h...@jakarta.apache.org > >> > >> > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@jakarta.apache.org > For additional commands, e-mail: dev-h...@jakarta.apache.org > > -- Cordialement. Philippe Mouawad.
