You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@johnzon.apache.org by Hendrik Dev <he...@gmail.com> on 2015/04/21 22:49:10 UTC
Few thoughts and questions on javax.json.spi.JsonProvider (geronimo spec)
A few thoughts and questions on
JsonProvider.doLoadProvider():
- "tccl" can be null (in case of system classloader) but thats never
really checked
- special handling org.apache.geronimo.osgi.locator.ProviderLocator
really needed here?
JsonProvider.provider():
- doPrivileged/SecurityManager check really needed here?
- method seems thread safe but we do not cache the returned provider
instance. Maybe we can to this in a thread local variable?
Thanks
Hendrik
--
Hendrik Saly (salyh, hendrikdev22)
@hendrikdev22
PGP: 0x22D7F6EC
Re: Few thoughts and questions on javax.json.spi.JsonProvider
(geronimo spec)
Posted by Romain Manni-Bucau <rm...@gmail.com>.
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" <he...@gmail.com> 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
> <rm...@gmail.com> wrote:
> > 2015-04-22 0:01 GMT+02:00 Hendrik Dev <he...@gmail.com>:
> >
> >> On Tue, Apr 21, 2015 at 11:06 PM, Romain Manni-Bucau
> >> <rm...@gmail.com> wrote:
> >> > Le 21 avr. 2015 22:51, "Hendrik Dev" <he...@gmail.com> 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
>
Re: Few thoughts and questions on javax.json.spi.JsonProvider
(geronimo spec)
Posted by Hendrik Dev <he...@gmail.com>.
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
<rm...@gmail.com> wrote:
> 2015-04-22 0:01 GMT+02:00 Hendrik Dev <he...@gmail.com>:
>
>> On Tue, Apr 21, 2015 at 11:06 PM, Romain Manni-Bucau
>> <rm...@gmail.com> wrote:
>> > Le 21 avr. 2015 22:51, "Hendrik Dev" <he...@gmail.com> 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
Re: Few thoughts and questions on javax.json.spi.JsonProvider
(geronimo spec)
Posted by Romain Manni-Bucau <rm...@gmail.com>.
2015-04-22 0:01 GMT+02:00 Hendrik Dev <he...@gmail.com>:
> On Tue, Apr 21, 2015 at 11:06 PM, Romain Manni-Bucau
> <rm...@gmail.com> wrote:
> > Le 21 avr. 2015 22:51, "Hendrik Dev" <he...@gmail.com> 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
>
Re: Few thoughts and questions on javax.json.spi.JsonProvider
(geronimo spec)
Posted by Hendrik Dev <he...@gmail.com>.
On Tue, Apr 21, 2015 at 11:06 PM, Romain Manni-Bucau
<rm...@gmail.com> wrote:
> Le 21 avr. 2015 22:51, "Hendrik Dev" <he...@gmail.com> 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
>
>> - special handling org.apache.geronimo.osgi.locator.ProviderLocator
>> really needed here?
>>
>
> In G spec jars yes.
OK
>
>> 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
>
> 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
Re: Few thoughts and questions on javax.json.spi.JsonProvider
(geronimo spec)
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le 21 avr. 2015 22:51, "Hendrik Dev" <he...@gmail.com> 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
> - special handling org.apache.geronimo.osgi.locator.ProviderLocator
> really needed here?
>
In G spec jars yes.
> JsonProvider.provider():
> - doPrivileged/SecurityManager check really needed here?
For containers yes and doesnt hurt at runtime normally.
> - 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.
Did you hit any issue?
> Thanks
> Hendrik
>
> --
> Hendrik Saly (salyh, hendrikdev22)
> @hendrikdev22
> PGP: 0x22D7F6EC