You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cocoon.apache.org by Thorsten Scherler <th...@juntadeandalucia.es> on 2008/08/21 14:18:03 UTC
[2.1] AbstractCachingProcessingPipeline and cocoon CLI
Hi all,
still updating forrest to use cocoon-2.1.x and I found a problem in the
AbstractCachingProcessingPipeline.
I am not sure whether someone is using the cocoon cli ATM. Forrest is
based around this component.
https://issues.apache.org/jira/browse/FOR-955?focusedCommentId=12624340#action_12624340
I found that in
org.apache.cocoon.components.pipeline.impl.AbstractCachingProcessingPipeline line 245
Object lock =
env.getObjectModel().get(HttpEnvironment.HTTP_REQUEST_OBJECT);
the lock is null which causes the NPE in the end.
I changed it the following way:
Object lock =
env.getObjectModel().get(HttpEnvironment.HTTP_REQUEST_OBJECT);
+ if (null==lock){
+ lock =
env.getObjectModel().get(ObjectModelHelper.REQUEST_OBJECT);
+ }
try {
and now it is working as before.
Can somebody verify that it is a bug? Should I commit the patch?
TIA for any feedback.
salu2
--
Thorsten Scherler thorsten.at.apache.org
Open Source Java consulting, training and solutions
Re: [2.1] AbstractCachingProcessingPipeline and cocoon CLI
Posted by Thorsten Scherler <th...@juntadeandalucia.es>.
On Fri, 2008-08-22 at 09:35 +0200, Thorsten Scherler wrote:
> On Thu, 2008-08-21 at 19:40 -0400, Vadim Gritsenko wrote:
> > On Aug 21, 2008, at 8:18 AM, Thorsten Scherler wrote:
> >
> > > still updating forrest to use cocoon-2.1.x and I found a problem in
> > > the
> > > AbstractCachingProcessingPipeline.
> > >
> > > I am not sure whether someone is using the cocoon cli ATM. Forrest is
> > > based around this component.
> > >
> > > https://issues.apache.org/jira/browse/FOR-955?focusedCommentId=12624340
> > > #action_12624340
...
> > > Can somebody verify that it is a bug? Should I commit the patch?
> >
> > It is a bug, but with proposed change you can get a deadlock under
> > some conditions (IIRC, when using parallels inclusion in the sitemap
> > which ultimately pull from the same pipeline). The idea behind that
> > lock is to synchronize on the main request (top most request), and not
> > on any of sub-request object created during aggregation. Suitable
> > alternative would be to lock against top most command line request.
>
> How about:
Index:
src/java/org/apache/cocoon/components/pipeline/impl/AbstractCachingProcessingPipeline.java
===================================================================
---
src/java/org/apache/cocoon/components/pipeline/impl/AbstractCachingProcessingPipeline.java (revision 681296)
+++
src/java/org/apache/cocoon/components/pipeline/impl/AbstractCachingProcessingPipeline.java (working copy)
@@ -36,7 +36,7 @@
import org.apache.cocoon.caching.ComponentCacheKey;
import org.apache.cocoon.caching.PipelineCacheKey;
import org.apache.cocoon.environment.Environment;
-import org.apache.cocoon.environment.http.HttpEnvironment;
+import org.apache.cocoon.environment.ObjectModelHelper;
import org.apache.cocoon.transformation.Transformer;
import org.apache.cocoon.util.HashUtil;
import org.apache.excalibur.source.SourceValidity;
@@ -192,7 +192,7 @@
}
// Avoid deadlock with self (see JIRA COCOON-1985).
- Object current =
env.getObjectModel().get(HttpEnvironment.HTTP_REQUEST_OBJECT);
+ Object current =
env.getObjectModel().get(ObjectModelHelper.REQUEST_OBJECT);
if (lock != null && lock != current) {
if (getLogger().isDebugEnabled()) {
getLogger().debug("Waiting on Lock '" + lockKey +
"'");
@@ -242,7 +242,7 @@
getLogger().debug("Lock EXISTS: '" + lockKey +
"'");
}
} else {
- Object lock =
env.getObjectModel().get(HttpEnvironment.HTTP_REQUEST_OBJECT);
+ Object lock =
env.getObjectModel().get(ObjectModelHelper.REQUEST_OBJECT);
try {
transientStore.store(lockKey, lock);
} catch (IOException e) {
This way we always lock the same object. I had a look as well on COCOON-1985 and it should be compatible.
Thanks for the feedback Vadim.
salu2
--
Thorsten Scherler thorsten.at.apache.org
Open Source Java consulting, training and solutions
Re: [2.1] AbstractCachingProcessingPipeline and cocoon CLI
Posted by Vadim Gritsenko <va...@reverycodes.com>.
On Aug 22, 2008, at 11:26 AM, Rainer Pruy wrote:
> Hi,
> sorry for intruding this discussion.....
Not intruding at all :)
> just from what I can take from this discussion:
>
> Wouldn't it be easier to enclose this into a generalized Environment
> implementation
> that does support Environment.getTopRequest() and is stored with
> ObjectModel in place of those different "REQUEST_OBJECT" instances -
> all different?
> That way you could also have some getCurrentRequest() or
> getNthRequest(int n) or any other operation that has to deal with
> request
> environments and knowledge about can be kept more local.
It is a nice suggestion but it has to be weighted in carefully with
regards to all Environment implementations in 2.1 and different
architecture in 2.2. Given my limited time, I'm not going to venture
into that. Goal of my comments was mainly to point out to the problem,
and not to suggest any particular solution. But you are certainly
welcome to work out the best way on how to handle this problem :)
Vadim
Re: [2.1] AbstractCachingProcessingPipeline and cocoon CLI
Posted by Rainer Pruy <Ra...@Acrys.COM>.
Hi,
sorry for intruding this discussion.....
just from what I can take from this discussion:
Wouldn't it be easier to enclose this into a generalized Environment implementation
that does support Environment.getTopRequest() and is stored with ObjectModel in place of those different "REQUEST_OBJECT" instances -
all different?
That way you could also have some getCurrentRequest() or getNthRequest(int n) or any other operation that has to deal with request
environments and knowledge about can be kept more local.
Just my 0,02 EUR
Rainer
Vadim Gritsenko schrieb:
> On Aug 22, 2008, at 8:57 AM, Thorsten Scherler wrote:
>
>> On Fri, 2008-08-22 at 08:18 -0400, Vadim Gritsenko wrote:
>>> On Aug 22, 2008, at 3:35 AM, Thorsten Scherler wrote:
>>>
>>>> How about:
>>>> } else {
>>>> - Object lock =
>>>> env.getObjectModel().get(HttpEnvironment.HTTP_REQUEST_OBJECT);
>>>> + Object lock =
>>>> env.getObjectModel().get(ObjectModelHelper.REQUEST_OBJECT);
>>>> try {
>>>> transientStore.store(lockKey, lock);
>>>> } catch (IOException e) {
>>>>
>>>> This way we always lock the same object.
>>>
>>> Exactly this thing I was trying to explain - it will not work because
>>> instead of using top level request you are suggesting to use current
>>> sub-request.
>>
>> Sorry I am not really following. As understand you we cannot use
>> ObjectModelHelper.REQUEST_OBJECT since we cannot be sure that is not a
>> sub-request (a request for the exact same resource in a concurrent
>> situation).
>
> Exactly.
>
>
>> If the lock is null for a HttpEnvironment.HTTP_REQUEST_OBJECT then we
>> are in CLI mode.
>
> (patch quoted above does not have lock == null comparison anymore,
> that's why it is dangerous for HTTP env)
>
>
>> In CLI ASAIK there is no concurrent situation meaning
>> the problem cannot occur. However since the issue is talking about
>> includes that may occur.
>
> Yes. You can still use IncludeTransformer in CLI. So deadlock can happen.
>
>
>> Having a typical call from the cli gives for the env.getObjectModel():
>>
>> HashMap values:
>> [null,
>> response=org.apache.cocoon.environment.wrapper.ResponseWrapper@fd4662,
>> link-collection=[],
>> request=org.apache.cocoon.environment.wrapper.RequestWrapper@6dddcf,
>> null, null, null, null,
>> context=org.apache.cocoon.environment.commandline.CommandLineContext@14ba9a2,
>> null, null,
>> source-resolver=org.apache.cocoon.environment.commandline.FileSavingEnvironment@725967,
>> org.apache.cocoon.components.CocoonComponentManager=org.apache.cocoon.components.EnvironmentDescription@d2efa1,
>> null, null, null]
>>
>> So which object you would suggest to lock?
>>
>> You wrote "Suitable alternative would be to lock against top most
>> command line request."
>>
>> To what you are referring with "top most command line request"?
>
> That would be CommandLineRequest. Following HTTP environment analogy,
> that would be CommandLineEnvironment.CLI_REQUEST_OBJECT.
>
>
> Vadim
>
>
>> salu2
>>
>>>
>>> Take a look at EnvironmentWrapper (which is used e.g. in SitemapSource):
>>>
>>> // create new object model and replace the request object
>>> Map oldObjectModel = env.getObjectModel();
>>> if (oldObjectModel instanceof HashMap) {
>>> this.objectModel = (Map)((HashMap)oldObjectModel).clone();
>>> } else {
>>> this.objectModel = new HashMap(oldObjectModel.size()*2);
>>> Iterator entries = oldObjectModel.entrySet().iterator();
>>> Map.Entry entry;
>>> while (entries.hasNext()) {
>>> entry = (Map.Entry)entries.next();
>>> this.objectModel.put(entry.getKey(), entry.getValue());
>>> }
>>> }
>>> this.request = new
>>> RequestWrapper(ObjectModelHelper.getRequest(oldObjectModel),
>>> requestURI,
>>> queryString,
>>> this,
>>> rawMode);
>>> this.objectModel.put(ObjectModelHelper.REQUEST_OBJECT,
>>> this.request);
>>>
>>>
>>> Vadim
>> --
>> Thorsten Scherler thorsten.at.apache.org
>> Open Source Java consulting, training and solutions
>>
>
Re: [2.1] AbstractCachingProcessingPipeline and cocoon CLI
Posted by Thorsten Scherler <th...@juntadeandalucia.es>.
On Thu, 2008-08-28 at 07:44 -0400, Vadim Gritsenko wrote:
> On Aug 26, 2008, at 3:59 AM, Thorsten Scherler wrote:
>
> > On Mon, 2008-08-25 at 20:23 -0400, Vadim Gritsenko wrote:
> >> On Aug 25, 2008, at 6:40 AM, Thorsten Scherler wrote:
> > ...
> >> Yes, ObjectModelHelper.REQUEST_OBJECT object is always unique. It is
> >> actually unique in any environment. And since it is unique, it does
> >> not make sense to lock on it at all - lock on unique object serves no
> >> purpose :-)
> >
> > Makes me wonder since HttpServletRequest is always unique as well
> > due to
> > the headers, so does not violate as well the whole locking?
>
> HttpServletRequest is unique only in context of external request; but
> it is same for multiple sub-requests of single external request.
> Similarly, CommandLineRequest would be unique per single external
> request, but shared across sub-requests.
>
> At the same time, ObjectModelHelper.REQUEST_OBJECT will be always
> unique: it will either be HttpServletRequest/CommandLineRequest for
> top level, or Wrapper for nested requests.
>
>
>
> >> You could, for example, have same effect with code:
> >>
> >> // Avoids NPE, does nothing
> >> if (lock == null) lock = new Object();
> >>
> >>
> >> To get back to the problem... The original goal of pipeline locking
> >> was to prevent separate external requests to start generating cached
> >> response for the same resource-intensive resource. In other words, if
> >> too external requests both (directly or through aggregation) hit '/
> >> slow/cached/foo' resource, only one request will trigger this
> >> pipeline, while another will wait for the first to complete.
> >>
> >> This logic, in turn, had to be augmented to exclude single external
> >> request hitting the same slow resource twice due to aggregation (and
> >> parallel aggregation), hence locking against top level external
> >> request was implemented.
> >>
> >
> > Understand but I am confused due to the above fact that the
> > HttpServletRequest is as well unique as it is my understanding.
> >
> >> Now, in situation when CommandLineEnvironment is used from the, ahem,
> >> command line, two external requests will not happen. Which means that
> >> whole pipeline locking thing is irrelevant and can be omitted
> >> completely (like in 'lock = new Object()' snippet above). But, the
> >> same CommandLineEnvironment stuff can be used in multi threaded
> >> environment which uses the CocoonBean class. So, strictly speaking,
> >> in
> >> this scenario locking still should be performed against external
> >> request object, and not against any RequestWrapper which is unique
> >> for
> >> each sub-request.
> >
> > I now are using the url as lock key since as I understand it perfectly
> > qualifies as lock key.
>
> Why not CommandLineRequest?
Hmm, because it would be the same as ObjectModelHelper.REQUEST_OBJECT
this.objectModel.put(ObjectModelHelper.REQUEST_OBJECT, new
CommandLineRequest(this, null, uri, null, attributes, parameters,
headers));
However if you recommend to do:
CommandLineRequest request = new CommandLineRequest(this, null, uri,
null, attributes, parameters, headers);
this.objectModel.put(ObjectModelHelper.REQUEST_OBJECT,
request);
this.objectModel.put(CLI_REQUEST, request);
That is as well fine with me.
wdyt?
Thanks for your feedback.
salu2
>
>
> > The patch is attached to COCOON-2241. As soon you are ok with it I
> > will
> > commit it.
>
> I'd rather use CommandLineRequest; or at least change one line to be:
>
> this.objectModel.put(CLI_REQUEST_ID, new String(uri));
>
>
>
> Vadim
>
>
> > Thanks for your patience.
> >
> > salu2
> >
> >>
> >> Hope now it all makes more sense.
> >>
> >> Regards,
> >> Vadim
> >>
> >>
> >>> In comparison the HttpEnvironment has in the constructor:
> >>> this.objectModel.put(HTTP_REQUEST_OBJECT, req);
> >>> where HttpServletRequest req.
> >>>
> >>> The uniqueness in both cases are, as I understand, the headers.
> >>>
> >>> salu2
> >>
> > --
> > Thorsten Scherler
> > thorsten.at.apache.org
> > Open Source Java consulting, training and
> > solutions
> >
>
--
Thorsten Scherler thorsten.at.apache.org
Open Source Java consulting, training and solutions
Re: [2.1] AbstractCachingProcessingPipeline and cocoon CLI
Posted by Vadim Gritsenko <va...@reverycodes.com>.
On Aug 26, 2008, at 3:59 AM, Thorsten Scherler wrote:
> On Mon, 2008-08-25 at 20:23 -0400, Vadim Gritsenko wrote:
>> On Aug 25, 2008, at 6:40 AM, Thorsten Scherler wrote:
> ...
>> Yes, ObjectModelHelper.REQUEST_OBJECT object is always unique. It is
>> actually unique in any environment. And since it is unique, it does
>> not make sense to lock on it at all - lock on unique object serves no
>> purpose :-)
>
> Makes me wonder since HttpServletRequest is always unique as well
> due to
> the headers, so does not violate as well the whole locking?
HttpServletRequest is unique only in context of external request; but
it is same for multiple sub-requests of single external request.
Similarly, CommandLineRequest would be unique per single external
request, but shared across sub-requests.
At the same time, ObjectModelHelper.REQUEST_OBJECT will be always
unique: it will either be HttpServletRequest/CommandLineRequest for
top level, or Wrapper for nested requests.
>> You could, for example, have same effect with code:
>>
>> // Avoids NPE, does nothing
>> if (lock == null) lock = new Object();
>>
>>
>> To get back to the problem... The original goal of pipeline locking
>> was to prevent separate external requests to start generating cached
>> response for the same resource-intensive resource. In other words, if
>> too external requests both (directly or through aggregation) hit '/
>> slow/cached/foo' resource, only one request will trigger this
>> pipeline, while another will wait for the first to complete.
>>
>> This logic, in turn, had to be augmented to exclude single external
>> request hitting the same slow resource twice due to aggregation (and
>> parallel aggregation), hence locking against top level external
>> request was implemented.
>>
>
> Understand but I am confused due to the above fact that the
> HttpServletRequest is as well unique as it is my understanding.
>
>> Now, in situation when CommandLineEnvironment is used from the, ahem,
>> command line, two external requests will not happen. Which means that
>> whole pipeline locking thing is irrelevant and can be omitted
>> completely (like in 'lock = new Object()' snippet above). But, the
>> same CommandLineEnvironment stuff can be used in multi threaded
>> environment which uses the CocoonBean class. So, strictly speaking,
>> in
>> this scenario locking still should be performed against external
>> request object, and not against any RequestWrapper which is unique
>> for
>> each sub-request.
>
> I now are using the url as lock key since as I understand it perfectly
> qualifies as lock key.
Why not CommandLineRequest?
> The patch is attached to COCOON-2241. As soon you are ok with it I
> will
> commit it.
I'd rather use CommandLineRequest; or at least change one line to be:
this.objectModel.put(CLI_REQUEST_ID, new String(uri));
Vadim
> Thanks for your patience.
>
> salu2
>
>>
>> Hope now it all makes more sense.
>>
>> Regards,
>> Vadim
>>
>>
>>> In comparison the HttpEnvironment has in the constructor:
>>> this.objectModel.put(HTTP_REQUEST_OBJECT, req);
>>> where HttpServletRequest req.
>>>
>>> The uniqueness in both cases are, as I understand, the headers.
>>>
>>> salu2
>>
> --
> Thorsten Scherler
> thorsten.at.apache.org
> Open Source Java consulting, training and
> solutions
>
Re: [2.1] AbstractCachingProcessingPipeline and cocoon CLI
Posted by Thorsten Scherler <th...@juntadeandalucia.es>.
On Mon, 2008-08-25 at 20:23 -0400, Vadim Gritsenko wrote:
> On Aug 25, 2008, at 6:40 AM, Thorsten Scherler wrote:
...
> Yes, ObjectModelHelper.REQUEST_OBJECT object is always unique. It is
> actually unique in any environment. And since it is unique, it does
> not make sense to lock on it at all - lock on unique object serves no
> purpose :-)
Makes me wonder since HttpServletRequest is always unique as well due to
the headers, so does not violate as well the whole locking?
> You could, for example, have same effect with code:
>
> // Avoids NPE, does nothing
> if (lock == null) lock = new Object();
>
>
> To get back to the problem... The original goal of pipeline locking
> was to prevent separate external requests to start generating cached
> response for the same resource-intensive resource. In other words, if
> too external requests both (directly or through aggregation) hit '/
> slow/cached/foo' resource, only one request will trigger this
> pipeline, while another will wait for the first to complete.
>
> This logic, in turn, had to be augmented to exclude single external
> request hitting the same slow resource twice due to aggregation (and
> parallel aggregation), hence locking against top level external
> request was implemented.
>
Understand but I am confused due to the above fact that the
HttpServletRequest is as well unique as it is my understanding.
> Now, in situation when CommandLineEnvironment is used from the, ahem,
> command line, two external requests will not happen. Which means that
> whole pipeline locking thing is irrelevant and can be omitted
> completely (like in 'lock = new Object()' snippet above). But, the
> same CommandLineEnvironment stuff can be used in multi threaded
> environment which uses the CocoonBean class. So, strictly speaking, in
> this scenario locking still should be performed against external
> request object, and not against any RequestWrapper which is unique for
> each sub-request.
I now are using the url as lock key since as I understand it perfectly
qualifies as lock key.
The patch is attached to COCOON-2241. As soon you are ok with it I will
commit it.
Thanks for your patience.
salu2
>
> Hope now it all makes more sense.
>
> Regards,
> Vadim
>
>
> > In comparison the HttpEnvironment has in the constructor:
> > this.objectModel.put(HTTP_REQUEST_OBJECT, req);
> > where HttpServletRequest req.
> >
> > The uniqueness in both cases are, as I understand, the headers.
> >
> > salu2
>
--
Thorsten Scherler thorsten.at.apache.org
Open Source Java consulting, training and solutions
Re: [2.1] AbstractCachingProcessingPipeline and cocoon CLI
Posted by Vadim Gritsenko <va...@reverycodes.com>.
On Aug 25, 2008, at 6:40 AM, Thorsten Scherler wrote:
> On Fri, 2008-08-22 at 10:34 -0400, Vadim Gritsenko wrote:
>> On Aug 22, 2008, at 8:57 AM, Thorsten Scherler wrote:
>>
>>> So which object you would suggest to lock?
>>>
>>> You wrote "Suitable alternative would be to lock against top most
>>> command line request."
>>>
>>> To what you are referring with "top most command line request"?
>>
>> That would be CommandLineRequest.
>
> Hmm, did you see the above objectModel? There is no such an object in
> the map. There are exactly six objects in the model
>
> 1) response
> 2) link-collection
> 3) request
> 4) context
> 5) source-resolver
> 6) org.apache.cocoon.components.CocoonComponentManager
Yes, it currently does not exist.
>> Following HTTP environment analogy,
>> that would be CommandLineEnvironment.CLI_REQUEST_OBJECT.
>
> Which does not exist, right?
Correct. At the moment, it does not exist.
> I opened an issue COCOON-2241 around the issue.
>
> The proposed solution is now taken the
> ObjectModelHelper.REQUEST_OBJECT
> if the request is from CLI, if not it is using the
> HttpEnvironment.HTTP_REQUEST_OBJECT.
>
> IMO it should not be a problem with deadlocks since the
> FileSavingEnvironment and LinkSamplingEnvironment (as only
> implementations for the Cli) have in the constructor:
>
> this.objectModel.put(ObjectModelHelper.REQUEST_OBJECT,
> new CommandLineRequest(this, null, uri, null, attributes, parameters,
> headers));
>
> which IMO makes it unique for each sub-request.
Yes, ObjectModelHelper.REQUEST_OBJECT object is always unique. It is
actually unique in any environment. And since it is unique, it does
not make sense to lock on it at all - lock on unique object serves no
purpose :-)
You could, for example, have same effect with code:
// Avoids NPE, does nothing
if (lock == null) lock = new Object();
To get back to the problem... The original goal of pipeline locking
was to prevent separate external requests to start generating cached
response for the same resource-intensive resource. In other words, if
too external requests both (directly or through aggregation) hit '/
slow/cached/foo' resource, only one request will trigger this
pipeline, while another will wait for the first to complete.
This logic, in turn, had to be augmented to exclude single external
request hitting the same slow resource twice due to aggregation (and
parallel aggregation), hence locking against top level external
request was implemented.
Now, in situation when CommandLineEnvironment is used from the, ahem,
command line, two external requests will not happen. Which means that
whole pipeline locking thing is irrelevant and can be omitted
completely (like in 'lock = new Object()' snippet above). But, the
same CommandLineEnvironment stuff can be used in multi threaded
environment which uses the CocoonBean class. So, strictly speaking, in
this scenario locking still should be performed against external
request object, and not against any RequestWrapper which is unique for
each sub-request.
Hope now it all makes more sense.
Regards,
Vadim
> In comparison the HttpEnvironment has in the constructor:
> this.objectModel.put(HTTP_REQUEST_OBJECT, req);
> where HttpServletRequest req.
>
> The uniqueness in both cases are, as I understand, the headers.
>
> salu2
Re: [2.1] AbstractCachingProcessingPipeline and cocoon CLI
Posted by Thorsten Scherler <th...@juntadeandalucia.es>.
On Fri, 2008-08-22 at 10:34 -0400, Vadim Gritsenko wrote:
> On Aug 22, 2008, at 8:57 AM, Thorsten Scherler wrote:
>
> > On Fri, 2008-08-22 at 08:18 -0400, Vadim Gritsenko wrote:
> >> On Aug 22, 2008, at 3:35 AM, Thorsten Scherler wrote:
> >>
> >>> How about:
> >>> } else {
> >>> - Object lock =
> >>> env.getObjectModel().get(HttpEnvironment.HTTP_REQUEST_OBJECT);
> >>> + Object lock =
> >>> env.getObjectModel().get(ObjectModelHelper.REQUEST_OBJECT);
> >>> try {
> >>> transientStore.store(lockKey, lock);
> >>> } catch (IOException e) {
> >>>
> >>> This way we always lock the same object.
> >>
> >> Exactly this thing I was trying to explain - it will not work because
> >> instead of using top level request you are suggesting to use current
> >> sub-request.
> >
> > Sorry I am not really following. As understand you we cannot use
> > ObjectModelHelper.REQUEST_OBJECT since we cannot be sure that is not a
> > sub-request (a request for the exact same resource in a concurrent
> > situation).
>
> Exactly.
>
>
> > If the lock is null for a HttpEnvironment.HTTP_REQUEST_OBJECT then we
> > are in CLI mode.
>
> (patch quoted above does not have lock == null comparison anymore,
> that's why it is dangerous for HTTP env)
>
>
> > In CLI ASAIK there is no concurrent situation meaning
> > the problem cannot occur. However since the issue is talking about
> > includes that may occur.
>
> Yes. You can still use IncludeTransformer in CLI. So deadlock can
> happen.
>
>
> > Having a typical call from the cli gives for the env.getObjectModel():
> >
> > HashMap values:
> > [null,
> > response=org.apache.cocoon.environment.wrapper.ResponseWrapper@fd4662,
> > link-collection=[],
> > request=org.apache.cocoon.environment.wrapper.RequestWrapper@6dddcf,
> > null, null, null, null,
> > context
> > =
> > org
> > .apache.cocoon.environment.commandline.CommandLineContext@14ba9a2,
> > null, null, source-
> > resolver
> > =
> > org
> > .apache.cocoon.environment.commandline.FileSavingEnvironment@725967,
> > org
> > .apache
> > .cocoon
> > .components
> > .CocoonComponentManager
> > =org.apache.cocoon.components.EnvironmentDescription@d2efa1, null,
> > null, null]
> >
> > So which object you would suggest to lock?
> >
> > You wrote "Suitable alternative would be to lock against top most
> > command line request."
> >
> > To what you are referring with "top most command line request"?
>
> That would be CommandLineRequest.
Hmm, did you see the above objectModel? There is no such an object in
the map. There are exactly six objects in the model
1) response
2) link-collection
3) request
4) context
5) source-resolver
6) org.apache.cocoon.components.CocoonComponentManager
> Following HTTP environment analogy,
> that would be CommandLineEnvironment.CLI_REQUEST_OBJECT.
Which does not exist, right?
I opened an issue COCOON-2241 around the issue.
The proposed solution is now taken the ObjectModelHelper.REQUEST_OBJECT
if the request is from CLI, if not it is using the
HttpEnvironment.HTTP_REQUEST_OBJECT.
IMO it should not be a problem with deadlocks since the
FileSavingEnvironment and LinkSamplingEnvironment (as only
implementations for the Cli) have in the constructor:
this.objectModel.put(ObjectModelHelper.REQUEST_OBJECT,
new CommandLineRequest(this, null, uri, null, attributes, parameters,
headers));
which IMO makes it unique for each sub-request.
In comparison the HttpEnvironment has in the constructor:
this.objectModel.put(HTTP_REQUEST_OBJECT, req);
where HttpServletRequest req.
The uniqueness in both cases are, as I understand, the headers.
salu2
>
>
> Vadim
>
>
> > salu2
> >
> >>
> >> Take a look at EnvironmentWrapper (which is used e.g. in
> >> SitemapSource):
> >>
> >> // create new object model and replace the request object
> >> Map oldObjectModel = env.getObjectModel();
> >> if (oldObjectModel instanceof HashMap) {
> >> this.objectModel = (Map)
> >> ((HashMap)oldObjectModel).clone();
> >> } else {
> >> this.objectModel = new HashMap(oldObjectModel.size()*2);
> >> Iterator entries = oldObjectModel.entrySet().iterator();
> >> Map.Entry entry;
> >> while (entries.hasNext()) {
> >> entry = (Map.Entry)entries.next();
> >> this.objectModel.put(entry.getKey(),
> >> entry.getValue());
> >> }
> >> }
> >> this.request = new
> >> RequestWrapper(ObjectModelHelper.getRequest(oldObjectModel),
> >> requestURI,
> >> queryString,
> >> this,
> >> rawMode);
> >> this.objectModel.put(ObjectModelHelper.REQUEST_OBJECT,
> >> this.request);
> >>
> >>
> >> Vadim
> > --
> > Thorsten Scherler
> > thorsten.at.apache.org
> > Open Source Java consulting, training and
> > solutions
> >
>
--
Thorsten Scherler thorsten.at.apache.org
Open Source Java consulting, training and solutions
Re: [2.1] AbstractCachingProcessingPipeline and cocoon CLI
Posted by Vadim Gritsenko <va...@reverycodes.com>.
On Aug 22, 2008, at 8:57 AM, Thorsten Scherler wrote:
> On Fri, 2008-08-22 at 08:18 -0400, Vadim Gritsenko wrote:
>> On Aug 22, 2008, at 3:35 AM, Thorsten Scherler wrote:
>>
>>> How about:
>>> } else {
>>> - Object lock =
>>> env.getObjectModel().get(HttpEnvironment.HTTP_REQUEST_OBJECT);
>>> + Object lock =
>>> env.getObjectModel().get(ObjectModelHelper.REQUEST_OBJECT);
>>> try {
>>> transientStore.store(lockKey, lock);
>>> } catch (IOException e) {
>>>
>>> This way we always lock the same object.
>>
>> Exactly this thing I was trying to explain - it will not work because
>> instead of using top level request you are suggesting to use current
>> sub-request.
>
> Sorry I am not really following. As understand you we cannot use
> ObjectModelHelper.REQUEST_OBJECT since we cannot be sure that is not a
> sub-request (a request for the exact same resource in a concurrent
> situation).
Exactly.
> If the lock is null for a HttpEnvironment.HTTP_REQUEST_OBJECT then we
> are in CLI mode.
(patch quoted above does not have lock == null comparison anymore,
that's why it is dangerous for HTTP env)
> In CLI ASAIK there is no concurrent situation meaning
> the problem cannot occur. However since the issue is talking about
> includes that may occur.
Yes. You can still use IncludeTransformer in CLI. So deadlock can
happen.
> Having a typical call from the cli gives for the env.getObjectModel():
>
> HashMap values:
> [null,
> response=org.apache.cocoon.environment.wrapper.ResponseWrapper@fd4662,
> link-collection=[],
> request=org.apache.cocoon.environment.wrapper.RequestWrapper@6dddcf,
> null, null, null, null,
> context
> =
> org
> .apache.cocoon.environment.commandline.CommandLineContext@14ba9a2,
> null, null, source-
> resolver
> =
> org
> .apache.cocoon.environment.commandline.FileSavingEnvironment@725967,
> org
> .apache
> .cocoon
> .components
> .CocoonComponentManager
> =org.apache.cocoon.components.EnvironmentDescription@d2efa1, null,
> null, null]
>
> So which object you would suggest to lock?
>
> You wrote "Suitable alternative would be to lock against top most
> command line request."
>
> To what you are referring with "top most command line request"?
That would be CommandLineRequest. Following HTTP environment analogy,
that would be CommandLineEnvironment.CLI_REQUEST_OBJECT.
Vadim
> salu2
>
>>
>> Take a look at EnvironmentWrapper (which is used e.g. in
>> SitemapSource):
>>
>> // create new object model and replace the request object
>> Map oldObjectModel = env.getObjectModel();
>> if (oldObjectModel instanceof HashMap) {
>> this.objectModel = (Map)
>> ((HashMap)oldObjectModel).clone();
>> } else {
>> this.objectModel = new HashMap(oldObjectModel.size()*2);
>> Iterator entries = oldObjectModel.entrySet().iterator();
>> Map.Entry entry;
>> while (entries.hasNext()) {
>> entry = (Map.Entry)entries.next();
>> this.objectModel.put(entry.getKey(),
>> entry.getValue());
>> }
>> }
>> this.request = new
>> RequestWrapper(ObjectModelHelper.getRequest(oldObjectModel),
>> requestURI,
>> queryString,
>> this,
>> rawMode);
>> this.objectModel.put(ObjectModelHelper.REQUEST_OBJECT,
>> this.request);
>>
>>
>> Vadim
> --
> Thorsten Scherler
> thorsten.at.apache.org
> Open Source Java consulting, training and
> solutions
>
Re: [2.1] AbstractCachingProcessingPipeline and cocoon CLI
Posted by Thorsten Scherler <th...@juntadeandalucia.es>.
On Fri, 2008-08-22 at 08:18 -0400, Vadim Gritsenko wrote:
> On Aug 22, 2008, at 3:35 AM, Thorsten Scherler wrote:
>
> > How about:
> > } else {
> > - Object lock =
> > env.getObjectModel().get(HttpEnvironment.HTTP_REQUEST_OBJECT);
> > + Object lock =
> > env.getObjectModel().get(ObjectModelHelper.REQUEST_OBJECT);
> > try {
> > transientStore.store(lockKey, lock);
> > } catch (IOException e) {
> >
> > This way we always lock the same object.
>
> Exactly this thing I was trying to explain - it will not work because
> instead of using top level request you are suggesting to use current
> sub-request.
Sorry I am not really following. As understand you we cannot use
ObjectModelHelper.REQUEST_OBJECT since we cannot be sure that is not a
sub-request (a request for the exact same resource in a concurrent
situation).
If the lock is null for a HttpEnvironment.HTTP_REQUEST_OBJECT then we
are in CLI mode. In CLI ASAIK there is no concurrent situation meaning
the problem cannot occur. However since the issue is talking about
includes that may occur.
Having a typical call from the cli gives for the env.getObjectModel():
HashMap values:
[null,
response=org.apache.cocoon.environment.wrapper.ResponseWrapper@fd4662,
link-collection=[],
request=org.apache.cocoon.environment.wrapper.RequestWrapper@6dddcf,
null, null, null, null,
context=org.apache.cocoon.environment.commandline.CommandLineContext@14ba9a2, null, null, source-resolver=org.apache.cocoon.environment.commandline.FileSavingEnvironment@725967, org.apache.cocoon.components.CocoonComponentManager=org.apache.cocoon.components.EnvironmentDescription@d2efa1, null, null, null]
So which object you would suggest to lock?
You wrote "Suitable alternative would be to lock against top most
command line request."
To what you are referring with "top most command line request"?
salu2
>
> Take a look at EnvironmentWrapper (which is used e.g. in SitemapSource):
>
> // create new object model and replace the request object
> Map oldObjectModel = env.getObjectModel();
> if (oldObjectModel instanceof HashMap) {
> this.objectModel = (Map)((HashMap)oldObjectModel).clone();
> } else {
> this.objectModel = new HashMap(oldObjectModel.size()*2);
> Iterator entries = oldObjectModel.entrySet().iterator();
> Map.Entry entry;
> while (entries.hasNext()) {
> entry = (Map.Entry)entries.next();
> this.objectModel.put(entry.getKey(), entry.getValue());
> }
> }
> this.request = new
> RequestWrapper(ObjectModelHelper.getRequest(oldObjectModel),
> requestURI,
> queryString,
> this,
> rawMode);
> this.objectModel.put(ObjectModelHelper.REQUEST_OBJECT,
> this.request);
>
>
> Vadim
--
Thorsten Scherler thorsten.at.apache.org
Open Source Java consulting, training and solutions
Re: [2.1] AbstractCachingProcessingPipeline and cocoon CLI
Posted by Vadim Gritsenko <va...@reverycodes.com>.
On Aug 22, 2008, at 3:35 AM, Thorsten Scherler wrote:
> How about:
> } else {
> - Object lock =
> env.getObjectModel().get(HttpEnvironment.HTTP_REQUEST_OBJECT);
> + Object lock =
> env.getObjectModel().get(ObjectModelHelper.REQUEST_OBJECT);
> try {
> transientStore.store(lockKey, lock);
> } catch (IOException e) {
>
> This way we always lock the same object.
Exactly this thing I was trying to explain - it will not work because
instead of using top level request you are suggesting to use current
sub-request.
Take a look at EnvironmentWrapper (which is used e.g. in SitemapSource):
// create new object model and replace the request object
Map oldObjectModel = env.getObjectModel();
if (oldObjectModel instanceof HashMap) {
this.objectModel = (Map)((HashMap)oldObjectModel).clone();
} else {
this.objectModel = new HashMap(oldObjectModel.size()*2);
Iterator entries = oldObjectModel.entrySet().iterator();
Map.Entry entry;
while (entries.hasNext()) {
entry = (Map.Entry)entries.next();
this.objectModel.put(entry.getKey(), entry.getValue());
}
}
this.request = new
RequestWrapper(ObjectModelHelper.getRequest(oldObjectModel),
requestURI,
queryString,
this,
rawMode);
this.objectModel.put(ObjectModelHelper.REQUEST_OBJECT,
this.request);
Vadim
Re: [2.1] AbstractCachingProcessingPipeline and cocoon CLI
Posted by Thorsten Scherler <th...@juntadeandalucia.es>.
On Thu, 2008-08-21 at 19:40 -0400, Vadim Gritsenko wrote:
> On Aug 21, 2008, at 8:18 AM, Thorsten Scherler wrote:
>
> > still updating forrest to use cocoon-2.1.x and I found a problem in
> > the
> > AbstractCachingProcessingPipeline.
> >
> > I am not sure whether someone is using the cocoon cli ATM. Forrest is
> > based around this component.
> >
> > https://issues.apache.org/jira/browse/FOR-955?focusedCommentId=12624340
> > #action_12624340
> >
> > I found that in
> > org
> > .apache
> > .cocoon.components.pipeline.impl.AbstractCachingProcessingPipeline
> > line 245
> > Object lock =
> > env.getObjectModel().get(HttpEnvironment.HTTP_REQUEST_OBJECT);
> > the lock is null which causes the NPE in the end.
> >
> > I changed it the following way:
> > Object lock =
> > env.getObjectModel().get(HttpEnvironment.HTTP_REQUEST_OBJECT);
> > + if (null==lock){
> > + lock =
> > env.getObjectModel().get(ObjectModelHelper.REQUEST_OBJECT);
> > + }
> > try {
> >
> > and now it is working as before.
> >
> > Can somebody verify that it is a bug? Should I commit the patch?
>
> It is a bug, but with proposed change you can get a deadlock under
> some conditions (IIRC, when using parallels inclusion in the sitemap
> which ultimately pull from the same pipeline). The idea behind that
> lock is to synchronize on the main request (top most request), and not
> on any of sub-request object created during aggregation. Suitable
> alternative would be to lock against top most command line request.
How about:
Index:
src/java/org/apache/cocoon/components/pipeline/impl/AbstractCachingProcessingPipeline.java
===================================================================
---
src/java/org/apache/cocoon/components/pipeline/impl/AbstractCachingProcessingPipeline.java (revision 681296)
+++
src/java/org/apache/cocoon/components/pipeline/impl/AbstractCachingProcessingPipeline.java (working copy)
@@ -36,6 +36,7 @@
import org.apache.cocoon.caching.ComponentCacheKey;
import org.apache.cocoon.caching.PipelineCacheKey;
import org.apache.cocoon.environment.Environment;
+import org.apache.cocoon.environment.ObjectModelHelper;
import org.apache.cocoon.environment.http.HttpEnvironment;
import org.apache.cocoon.transformation.Transformer;
import org.apache.cocoon.util.HashUtil;
@@ -242,7 +243,7 @@
getLogger().debug("Lock EXISTS: '" + lockKey +
"'");
}
} else {
- Object lock =
env.getObjectModel().get(HttpEnvironment.HTTP_REQUEST_OBJECT);
+ Object lock =
env.getObjectModel().get(ObjectModelHelper.REQUEST_OBJECT);
try {
transientStore.store(lockKey, lock);
} catch (IOException e) {
This way we always lock the same object.
Thanks for the feedback Vadim.
salu2
>
> Vadim
--
Thorsten Scherler thorsten.at.apache.org
Open Source Java consulting, training and solutions
Re: [2.1] AbstractCachingProcessingPipeline and cocoon CLI
Posted by Vadim Gritsenko <va...@reverycodes.com>.
On Aug 21, 2008, at 8:18 AM, Thorsten Scherler wrote:
> still updating forrest to use cocoon-2.1.x and I found a problem in
> the
> AbstractCachingProcessingPipeline.
>
> I am not sure whether someone is using the cocoon cli ATM. Forrest is
> based around this component.
>
> https://issues.apache.org/jira/browse/FOR-955?focusedCommentId=12624340
> #action_12624340
>
> I found that in
> org
> .apache
> .cocoon.components.pipeline.impl.AbstractCachingProcessingPipeline
> line 245
> Object lock =
> env.getObjectModel().get(HttpEnvironment.HTTP_REQUEST_OBJECT);
> the lock is null which causes the NPE in the end.
>
> I changed it the following way:
> Object lock =
> env.getObjectModel().get(HttpEnvironment.HTTP_REQUEST_OBJECT);
> + if (null==lock){
> + lock =
> env.getObjectModel().get(ObjectModelHelper.REQUEST_OBJECT);
> + }
> try {
>
> and now it is working as before.
>
> Can somebody verify that it is a bug? Should I commit the patch?
It is a bug, but with proposed change you can get a deadlock under
some conditions (IIRC, when using parallels inclusion in the sitemap
which ultimately pull from the same pipeline). The idea behind that
lock is to synchronize on the main request (top most request), and not
on any of sub-request object created during aggregation. Suitable
alternative would be to lock against top most command line request.
Vadim