You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by Stefan Seifert <ss...@pro-vision.de> on 2014/10/22 10:22:24 UTC
[RTC] ThreadLocal for getting current request in sling
i propose to add a new feature to the Sling API and Sling Engine to access to the current request via an OSGi service, using a servlet filter and a thread local internally.
a proposal for such an implementation is currently part of sling models trunk, but should be renamed and moved to a more central part if we agree if it is a good idea. interface [1], impl [2].
we have already a comparable threadlocal concept for resource resolver [3]
my current usecases are:
- having the ability to inject context objects like request in any sling model, regardless of the adaptable. this is e.g. importing when adapting from a resource, but in context of a request (SLING-4083)
- configuration parameter override provider which injects overrides from HTTP header for test environments [4]
stefan
p.s. as pointed out by justin this topic was discussed already in the past, e.g. as a side-aspect of this thread [5]. this thread contains some more usecaes.
[1] https://svn.apache.org/repos/asf/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/SlingObjectInjectorRequestContext.java
[2] https://svn.apache.org/repos/asf/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/SlingObjectInjectorRequestContextFilter.java
[3] http://sling.apache.org/apidocs/sling7/org/apache/sling/api/resource/ResourceResolverFactory.html#getThreadResourceResolver--
[4] https://github.com/wcm-io/wcm-io/blob/master/config/core/src/main/java/io/wcm/config/core/override/impl/RequestHeaderOverrideProvider.java
[5] http://sling.markmail.org/thread/epn5vdw3fkmpsk6w
Re: [RTC] ThreadLocal for getting current request in sling
Posted by Carsten Ziegeler <cz...@apache.org>.
Am 22.10.14 um 10:52 schrieb Bertrand Delacretaz:
> Hi,
>
> On Wed, Oct 22, 2014 at 10:22 AM, Stefan Seifert <ss...@pro-vision.de> wrote:
>> ...i propose to add a new feature to the Sling API and Sling Engine to access to the current request via an OSGi service, using
>> a servlet filter and a thread local internally....
>
> +1 in principle, though as Carsten says we can implement this in the
> engine to avoid the filter.
>
>> ...we have already a comparable threadlocal concept for resource resolver [3]...
>
> So maybe we should rather put a single"request context" object in a
> ThreadLocal, instead of having different objects manage their own
> thread local things?
>
> Something like
>
> public interface ThreadLocalContext {
> SlingHttpServletRequest getCurrentRequest();
> ResourceResolver getCurrentResourceResolver();
> ResourceResolver [] getActiveResourceResolvers();
> }
>
The resource resolver thread local works outside of a request, so I dont
think we should deprecate that and replace it with the above mechanism.
Let's keep things separate.
Carsten
> etc...we can then test and evolve this in a cleaner way than with code
> scattered around various classes.
>
> For backwards compatibility, we can deprecate [3] and let it use this
> new API for now.
>
> -Bertrand
>
> [3] http://sling.apache.org/apidocs/sling7/org/apache/sling/api/resource/ResourceResolverFactory.html#getThreadResourceResolver--
>
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
Re: [RTC] ThreadLocal for getting current request in sling
Posted by Bertrand Delacretaz <bd...@apache.org>.
Hi,
On Wed, Oct 22, 2014 at 10:22 AM, Stefan Seifert <ss...@pro-vision.de> wrote:
> ...i propose to add a new feature to the Sling API and Sling Engine to access to the current request via an OSGi service, using
> a servlet filter and a thread local internally....
+1 in principle, though as Carsten says we can implement this in the
engine to avoid the filter.
> ...we have already a comparable threadlocal concept for resource resolver [3]...
So maybe we should rather put a single"request context" object in a
ThreadLocal, instead of having different objects manage their own
thread local things?
Something like
public interface ThreadLocalContext {
SlingHttpServletRequest getCurrentRequest();
ResourceResolver getCurrentResourceResolver();
ResourceResolver [] getActiveResourceResolvers();
}
etc...we can then test and evolve this in a cleaner way than with code
scattered around various classes.
For backwards compatibility, we can deprecate [3] and let it use this
new API for now.
-Bertrand
[3] http://sling.apache.org/apidocs/sling7/org/apache/sling/api/resource/ResourceResolverFactory.html#getThreadResourceResolver--
Re: [RTC] ThreadLocal for getting current request in sling
Posted by Carsten Ziegeler <cz...@apache.org>.
Am 22.10.14 um 10:22 schrieb Stefan Seifert:
> i propose to add a new feature to the Sling API and Sling Engine to access to the current request via an OSGi service, using a servlet filter and a thread local internally.
I think *if* we're doing this, we should add this into the engine and
avoid the filter completely. The request object changes for every
include and the engine is handling this already, so adding the code
there should be simple.
Carsten
>
> a proposal for such an implementation is currently part of sling models trunk, but should be renamed and moved to a more central part if we agree if it is a good idea. interface [1], impl [2].
>
> we have already a comparable threadlocal concept for resource resolver [3]
>
> my current usecases are:
> - having the ability to inject context objects like request in any sling model, regardless of the adaptable. this is e.g. importing when adapting from a resource, but in context of a request (SLING-4083)
> - configuration parameter override provider which injects overrides from HTTP header for test environments [4]
>
> stefan
>
> p.s. as pointed out by justin this topic was discussed already in the past, e.g. as a side-aspect of this thread [5]. this thread contains some more usecaes.
>
>
> [1] https://svn.apache.org/repos/asf/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/SlingObjectInjectorRequestContext.java
>
> [2] https://svn.apache.org/repos/asf/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/SlingObjectInjectorRequestContextFilter.java
>
> [3] http://sling.apache.org/apidocs/sling7/org/apache/sling/api/resource/ResourceResolverFactory.html#getThreadResourceResolver--
>
> [4] https://github.com/wcm-io/wcm-io/blob/master/config/core/src/main/java/io/wcm/config/core/override/impl/RequestHeaderOverrideProvider.java
>
> [5] http://sling.markmail.org/thread/epn5vdw3fkmpsk6w
>
>
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
RE: [RTC] ThreadLocal for getting current request in sling
Posted by Stefan Seifert <ss...@pro-vision.de>.
>> my current usecases are:
>> - having the ability to inject context objects like request in any sling
>model, regardless of the adaptable. this is e.g. importing when adapting from
>a resource, but in context of a request (SLING-4083)
>
>That should be possible to solve differently, such injection sounds like too
>much magic to me. Why can't sling models have a clear API that passes in such
>objects when they are created instead of going through a threadlocal?
sling models is based on a very simple API: adaptTo().
adaptTo passes in only the adaptable, and if this is not the request object there is no chance for the sling models implementation to get hold of it besides some "not-so-clean" way using a threadlocal.
but it's not always possible nor desired to just use the request as adaptable. e.g. if you handle multiple different resources inside your request and adapt them to a model and need access to the request there is no other way i can think of.
if fully agree that this way to access a request should not be the "standard way", and in most usecases there should be a better way (although not in the wcm.io configuration example because it as well relies on adaptTo(), but do not discuss this here in this thread further). SlingObjectInjectorRequestContext was only an name internal to the implementation of sling models.
stefan
Re: [RTC] ThreadLocal for getting current request in sling
Posted by Justin Edelson <ju...@justinedelson.com>.
On Wed, Oct 22, 2014 at 5:44 PM, Alexander Klimetschek
<ak...@adobe.com> wrote:
> On 22.10.2014, at 14:36, Justin Edelson <ju...@justinedelson.com> wrote:
>> To be clear, Sling Models was designed *not* to have an explicit API
>> and to be a way to implement of AdapterFactories. For version 1.2.0,
>> it does have an API, but this is essentially just a mirror of the
>> AdapterManager API plus some utility methods.
>
> On 22.10.2014, at 14:34, Stefan Seifert <ss...@pro-vision.de> wrote:
>> sling models is based on a very simple API: adaptTo().
>>
>> adaptTo passes in only the adaptable, and if this is not the request object there is no chance for the sling models implementation to get hold of it besides some "not-so-clean" way using a threadlocal.
>
> Maybe that's the problem and sling models should have a better way to be created instead of piggy-backing on the magic adaptTo().
Except what Stefan is highlighting is actually a generalized problem
with the adapter subsystem and not at all specific to Sling Models.
His use case could just as easily be rephased as "I want to adapt an
instance of Foo and in my AdapterFactory get to Bar (which is not
navigable from Foo), but I don't want the caller to have to worry
about this.", the outcome would be the same.
The only distinction is that these particular AdapterFactory
implementations are generated by Sling Models. This, however, is a
distinction without difference.
Personally, I don't quite see the use case -- if your AdapterFactory
(whether it is generated by Sling Models or hand-coded) needs
request-based context, the adaptable is the request.
>
> For JSPs: a taglib. For Java services: a service with methods like getModel(type, request) and getModel(type, resolver) etc. Imagine there is no ThreadLocal and adaptTo() available, how would you solve it in the most convenient fashion?
Such an API now exists, but I don't see how it changes this
discussion. The only way I see this API helping the use case is to
pass some kind of context object into the getModel(), e.g.
getModel(type, resource, Collections.singletonMap("request",
request)). Is this along the lines of what you are suggesting? And
again, if the caller knows they need to pass in the request, then the
adaptable should be the request.
Regards,
Justin
>
> Cheers,
> Alex
Re: [RTC] ThreadLocal for getting current request in sling
Posted by Alexander Klimetschek <ak...@adobe.com>.
On 22.10.2014, at 14:36, Justin Edelson <ju...@justinedelson.com> wrote:
> To be clear, Sling Models was designed *not* to have an explicit API
> and to be a way to implement of AdapterFactories. For version 1.2.0,
> it does have an API, but this is essentially just a mirror of the
> AdapterManager API plus some utility methods.
On 22.10.2014, at 14:34, Stefan Seifert <ss...@pro-vision.de> wrote:
> sling models is based on a very simple API: adaptTo().
>
> adaptTo passes in only the adaptable, and if this is not the request object there is no chance for the sling models implementation to get hold of it besides some "not-so-clean" way using a threadlocal.
Maybe that's the problem and sling models should have a better way to be created instead of piggy-backing on the magic adaptTo().
For JSPs: a taglib. For Java services: a service with methods like getModel(type, request) and getModel(type, resolver) etc. Imagine there is no ThreadLocal and adaptTo() available, how would you solve it in the most convenient fashion?
Cheers,
Alex
Re: [RTC] ThreadLocal for getting current request in sling
Posted by Justin Edelson <ju...@justinedelson.com>.
Hi,
On Wed, Oct 22, 2014 at 5:21 PM, Alexander Klimetschek
<ak...@adobe.com> wrote:
> On 22.10.2014, at 01:22, Stefan Seifert <ss...@pro-vision.de> wrote:
>> my current usecases are:
>> - having the ability to inject context objects like request in any sling model, regardless of the adaptable. this is e.g. importing when adapting from a resource, but in context of a request (SLING-4083)
>
> That should be possible to solve differently, such injection sounds like too much magic to me. Why can't sling models have a clear API that passes in such objects when they are created instead of going through a threadlocal?
To be clear, Sling Models was designed *not* to have an explicit API
and to be a way to implement of AdapterFactories. For version 1.2.0,
it does have an API, but this is essentially just a mirror of the
AdapterManager API plus some utility methods.
"Such objects" are passed in currently, assuming they are navigable
from the adaptable and this is documented in
http://sling.staging.apache.org/documentation/bundles/models.html#available-injectors.
But Stefan is describing a use case where those objects are not
navigable.
Justin
Re: [RTC] ThreadLocal for getting current request in sling
Posted by Bertrand Delacretaz <bd...@apache.org>.
Hi,
On Thu, Oct 23, 2014 at 10:04 AM, Dominik Süß <do...@gmail.com> wrote:
> ...To get around that I was thinking if it wouldn't be a good idea to add a
> further adaptTo() signature which would allow to add a generic payload
> which could contain contextobjects that an adapterFactory could use for
> construction..
Reading this thread I was thinking the same - we could add a second
"context" parameter to adaptTo(), that's just an Object that the
adapter is supposed to understand.
-Bertrand
Re: [RTC] ThreadLocal for getting current request in sling
Posted by Alexander Klimetschek <ak...@adobe.com>.
On 23.10.2014, at 15:39, Alexander Klimetschek <ak...@adobe.com> wrote:
> BTW, this is what I think is great about OSGi services, that they are based on plain Java interfaces and don't have all the complexities of DI frameworks such as Sling
I mean "Spring" of course :)
Alex
Re: [RTC] ThreadLocal for getting current request in sling
Posted by Alexander Klimetschek <ak...@adobe.com>.
On 23.10.2014, at 15:14, Dominik Süß <do...@gmail.com> wrote:
>
> adding some contexobject is IMHO not hidding anything.
It is - if you never see that behind the scenes a request object is passed along (and what not else), but in other cases outside the request scope it is not, this can be confusing. It is important to be aware of the request scope - and many folks are easily fooled by that (I know, not you and the others discussing this here).
> There are cases
> where you would want to have the context for creation of a model (or any
> other adaptable) - so at construction time which disqualifieds some method
> to be called afterwards.
> What't the issue with someresource.adaptTo(MyClass.class, request) (or as
> bertrand mentioned any other payload) which could be used by the
> AdapterFactory?
Maybe, but I'd be reluctant to extend the adaptTo API in this way. (It might be very hard to extend that API, since so many code implements Adaptable).
> In case of Scripting languages as sightly we cannot easily create wrapper
> objects that are used for adaption that could hold the real adaptable and
> the additional context object(s).
You model could have a "RequestScoped" or some better named interface with a setRequest() method, which the sightly executor would cast to and use.
Just plain Java OO & interfaces... BTW, this is what I think is great about OSGi services, that they are based on plain Java interfaces and don't have all the complexities of DI frameworks such as Sling that try to do everything with magic behind the scenes.
Cheers,
Alex
Re: [RTC] ThreadLocal for getting current request in sling
Posted by Dominik Süß <do...@gmail.com>.
Hi Alex,
adding some contexobject is IMHO not hidding anything. There are cases
where you would want to have the context for creation of a model (or any
other adaptable) - so at construction time which disqualifieds some method
to be called afterwards.
What't the issue with someresource.adaptTo(MyClass.class, request) (or as
bertrand mentioned any other payload) which could be used by the
AdapterFactory?
In case of Scripting languages as sightly we cannot easily create wrapper
objects that are used for adaption that could hold the real adaptable and
the additional context object(s). Therefore I would love to have this
option and extend the Sightly use to be able to hand over an additional
object and/or by default trying to add the request object if adapted from a
model. The Sling Model or adaptable could then utilize this request object
or simply ignore it where not necessary.
Best regards
Dominik
On Thu, Oct 23, 2014 at 10:50 PM, Alexander Klimetschek <ak...@adobe.com>
wrote:
> On 23.10.2014, at 01:04, Dominik Süß <do...@gmail.com> wrote:
> >
> > While request.adaptTo(Target.class) would enable an AdapterFactory to get
> > hold of the currentResource there is no chance to get the the custom
> > resource.
>
> Why not have a setter?
>
> MyModel model = resource.adaptTo(MyModel.class)'
> model.setRequest(slingRequest);
>
> or just pass it along with the methods you call, if you only need it in
> some calls:
>
> model.doSomething("foo", slingRequest);
>
>
> IMO it is very important that devs using code are aware that they are
> using the request object and are in a request scope or not, so hiding it is
> not something you want to strive for.
>
> Cheers,
> Alex
>
>
Re: [RTC] ThreadLocal for getting current request in sling
Posted by Alexander Klimetschek <ak...@adobe.com>.
On 23.10.2014, at 01:04, Dominik Süß <do...@gmail.com> wrote:
>
> While request.adaptTo(Target.class) would enable an AdapterFactory to get
> hold of the currentResource there is no chance to get the the custom
> resource.
Why not have a setter?
MyModel model = resource.adaptTo(MyModel.class)'
model.setRequest(slingRequest);
or just pass it along with the methods you call, if you only need it in some calls:
model.doSomething("foo", slingRequest);
IMO it is very important that devs using code are aware that they are using the request object and are in a request scope or not, so hiding it is not something you want to strive for.
Cheers,
Alex
Re: [RTC] ThreadLocal for getting current request in sling
Posted by Dominik Süß <do...@gmail.com>.
Hi everyone,
didn't follow the whole discussion so I might have missed some poin but if
I get it right the case where it gets problematic is when I essentially
want to adapt to from a resource (that is not the currentResource) without
losing the request scope.
While request.adaptTo(Target.class) would enable an AdapterFactory to get
hold of the currentResource there is no chance to get the the custom
resource.
To get around that I was thinking if it wouldn't be a good idea to add a
further adaptTo() signature which would allow to add a generic payload
which could contain contextobjects that an adapterFactory could use for
construction.In case of SlingModels everything in this payload could be
injected if there is a matching property to inject those.
I know its a bold idea but much less magic then ThreadLocals.
Cheers
Dominik
On Thu, Oct 23, 2014 at 8:22 AM, Carsten Ziegeler <cz...@apache.org>
wrote:
> Am 22.10.14 um 23:21 schrieb Alexander Klimetschek:
> > On 22.10.2014, at 01:22, Stefan Seifert <ss...@pro-vision.de> wrote:
> >> i propose to add a new feature to the Sling API and Sling Engine to
> access to the current request via an OSGi service, using a servlet filter
> and a thread local internally.
> >
> > -1
> >
> >> a proposal for such an implementation is currently part of sling models
> trunk, but should be renamed and moved to a more central part if we agree
> if it is a good idea. interface [1], impl [2].
> >
> > Why? This looks like a hack to me, and will lead to people not designing
> their APIs properly (e.g. passing the request through). I see ThreadLocal
> as a workaround when you don't have influence on the API but still need to
> pass things around. But in this case (sling models and the wcm.io config,
> below) there is full control over the API.
> >
> > A class like SlingObjectInjectorRequestContext sound very much like
> anti-patterns from Spring et al.
> >
> >> we have already a comparable threadlocal concept for resource resolver
> [3]
> >
> > Broken window :D
>
> Well, it's easy to judge on a high level without knowing the details.
> We're all aware of the dangers of thread locals, but matter of fact is,
> there are use cases where you need the resource resolver and there are
> tons of layers in between the code using the resource resolver and where
> it is available. In some cases the api is not even created in Sling, so
> there is absolutely no way to pass the resolver down to the code other
> than using a thread local.
>
> And instead of creating an island solution for every use case, so we end
> up with dozens of thread locals and filters and whatnot, having a
> general single solution is imho preferable.
>
> However, I see/have use cases for the resource resolver, I'm not 100%
> about the request yet :)
>
> Regards
> Carsten
> --
> Carsten Ziegeler
> Adobe Research Switzerland
> cziegeler@apache.org
>
Re: [RTC] ThreadLocal for getting current request in sling
Posted by Carsten Ziegeler <cz...@apache.org>.
Am 22.10.14 um 23:21 schrieb Alexander Klimetschek:
> On 22.10.2014, at 01:22, Stefan Seifert <ss...@pro-vision.de> wrote:
>> i propose to add a new feature to the Sling API and Sling Engine to access to the current request via an OSGi service, using a servlet filter and a thread local internally.
>
> -1
>
>> a proposal for such an implementation is currently part of sling models trunk, but should be renamed and moved to a more central part if we agree if it is a good idea. interface [1], impl [2].
>
> Why? This looks like a hack to me, and will lead to people not designing their APIs properly (e.g. passing the request through). I see ThreadLocal as a workaround when you don't have influence on the API but still need to pass things around. But in this case (sling models and the wcm.io config, below) there is full control over the API.
>
> A class like SlingObjectInjectorRequestContext sound very much like anti-patterns from Spring et al.
>
>> we have already a comparable threadlocal concept for resource resolver [3]
>
> Broken window :D
Well, it's easy to judge on a high level without knowing the details.
We're all aware of the dangers of thread locals, but matter of fact is,
there are use cases where you need the resource resolver and there are
tons of layers in between the code using the resource resolver and where
it is available. In some cases the api is not even created in Sling, so
there is absolutely no way to pass the resolver down to the code other
than using a thread local.
And instead of creating an island solution for every use case, so we end
up with dozens of thread locals and filters and whatnot, having a
general single solution is imho preferable.
However, I see/have use cases for the resource resolver, I'm not 100%
about the request yet :)
Regards
Carsten
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
Re: [RTC] ThreadLocal for getting current request in sling
Posted by Alexander Klimetschek <ak...@adobe.com>.
On 22.10.2014, at 01:22, Stefan Seifert <ss...@pro-vision.de> wrote:
> i propose to add a new feature to the Sling API and Sling Engine to access to the current request via an OSGi service, using a servlet filter and a thread local internally.
-1
> a proposal for such an implementation is currently part of sling models trunk, but should be renamed and moved to a more central part if we agree if it is a good idea. interface [1], impl [2].
Why? This looks like a hack to me, and will lead to people not designing their APIs properly (e.g. passing the request through). I see ThreadLocal as a workaround when you don't have influence on the API but still need to pass things around. But in this case (sling models and the wcm.io config, below) there is full control over the API.
A class like SlingObjectInjectorRequestContext sound very much like anti-patterns from Spring et al.
> we have already a comparable threadlocal concept for resource resolver [3]
Broken window :D
> my current usecases are:
> - having the ability to inject context objects like request in any sling model, regardless of the adaptable. this is e.g. importing when adapting from a resource, but in context of a request (SLING-4083)
That should be possible to solve differently, such injection sounds like too much magic to me. Why can't sling models have a clear API that passes in such objects when they are created instead of going through a threadlocal?
> - configuration parameter override provider which injects overrides from HTTP header for test environments [4]
This shows that wcm.io is doing too much magic... also, what do you mean by test environments? That seems very specific, not sure if that validates such a change in the main sling api.
> [5] http://sling.markmail.org/thread/epn5vdw3fkmpsk6w
Those use cases fall under the "have to use ThreadLocal because we can't change the API", so IMO don't warrant to put that into Sling API, it should be hidden in those modules that need to do the ThreadLocal workaround for their integration.
Citing Bertrand from that thread:
"Passing the request to a method clearly indicates that you expect that
method to deal with it. It's a few more characters to type, but it
makes things clear.
IMHO, it's too easy to create a mess with threadlocals, which are just
another kind of global variables."
Cheers,
Alex