Thus lookup is slow so that is a way to cache it and fine while not in previous case - which is out of ee spec but comes from real life.
That said you create your factory once in general so i prefer to not cache it. Le 22 avr. 2015 12:59, "Hendrik Dev" <[email protected]> a écrit : > ok, but just wondering why the RI does threadlocal caching if its not > working within containers > > On Wed, Apr 22, 2015 at 9:06 AM, Romain Manni-Bucau > <[email protected]> wrote: > > 2015-04-22 0:01 GMT+02:00 Hendrik Dev <[email protected]>: > > > >> On Tue, Apr 21, 2015 at 11:06 PM, Romain Manni-Bucau > >> <[email protected]> wrote: > >> > Le 21 avr. 2015 22:51, "Hendrik Dev" <[email protected]> a > écrit : > >> >> > >> >> A few thoughts and questions on > >> >> > >> >> JsonProvider.doLoadProvider(): > >> >> - "tccl" can be null (in case of system classloader) but thats never > >> >> really checked > >> > > >> > If so johnzon cant be loaded isnt it? So not a big deal imo > >> > >> someone could set the context classloader explicitly to null, see > attached > >> diff > >> > >> > > Hmm, thought it was already working thanks to cl system call but nothing > > against this part of the patch. > > > > > >> > > >> >> - special handling org.apache.geronimo.osgi.locator.ProviderLocator > >> >> really needed here? > >> >> > >> > > >> > In G spec jars yes. > >> > >> OK > >> > >> > > In your patch you "forName" it in a static fashion, this shouldnt be the > > case to support overriding + OSGi correctly. A side note I completely > > missed too: should use tccl.loadClass and not Class.forName cause of > OSGi. > > > > > >> > > >> >> JsonProvider.provider(): > >> >> - doPrivileged/SecurityManager check really needed here? > >> > > >> > For containers yes and doesnt hurt at runtime normally. > >> > >> OK > >> > >> > > >> >> - method seems thread safe but we do not cache the returned provider > >> >> instance. Maybe we can to this in a thread local variable? > >> >> > >> > > >> > Not cached for container case + i dont expect it to be called often. > >> > > >> > Thread local would break ears or wars if johnzon is in one war, > jackson > >> in > >> > another and api in the container for instance + it would leak on > >> undeploy. > >> > >> i see, so maybe caching the hole provider, see attached diff > >> > >> > > -1, was my first impl but if you have this deployment (tree classloader > but > > OSGi would make it worse): > > > > container loader > > `- geronimo-json > > |- webapp 1 > > | `- johnzon-0.4 > > `- webapp 2 > > `- johnzon-0.7 > > > > Then you would leak the provider (impl) in the container. > > > > Now think to OSGi...headache ;) > > > > > >> > > >> > Did you hit any issue? > >> > >> nothing special, originally working on that to make sure that Johnzon > >> is working under OSGi (Karaf), so i looked at the RI and johnzon > >> specs. > >> > >> > > >> >> Thanks > >> >> Hendrik > >> >> > >> >> -- > >> >> Hendrik Saly (salyh, hendrikdev22) > >> >> @hendrikdev22 > >> >> PGP: 0x22D7F6EC > >> > >> > >> > >> -- > >> Hendrik Saly (salyh, hendrikdev22) > >> @hendrikdev22 > >> PGP: 0x22D7F6EC > >> > > > > -- > Hendrik Saly (salyh, hendrikdev22) > @hendrikdev22 > PGP: 0x22D7F6EC >
