You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Violeta Georgieva <mi...@gmail.com> on 2013/05/28 20:51:28 UTC
AsyncContext.dispatch(path) invoked more than once
Hi,
In the AsyncContext.dispatch(path) javadoc we have:
"There can be at most one asynchronous dispatch operation per asynchronous
cycle, which is started by a call to one of the
ServletRequest.startAsync()<file:///C:/vily/my%20documents/servlet-3_0-final-javadoc/javax/servlet/ServletRequest.html#startAsync()>methods.
Any attempt to perform an additional asynchronous dispatch
operation within the same asynchronous cycle will result in an
IllegalStateException. If startAsync is subsequently called on the
dispatched request, then any of the dispatch or
complete()<file:///C:/vily/my%20documents/servlet-3_0-final-javadoc/javax/servlet/AsyncContext.html#complete()>methods
may be called."
If we have the following scenario:
AsyncContext asyncContext = request.startAsync(request, response);
asyncContext .dispatch("/resourceA");
asyncContext .dispatch("/resourceB");
I would assume that the second dispatch will throw ISE and a dispatching to
the "resourceA" will happen.
The current implementation (tomcat7/8) throws ISE but the dispatching is to
the "resourceB".
I prepared a change (below) that moves the check for ISE a little bit
earlier. o.a.catalina.core.AsyncContextImpl.dispatch field is not
overridden with information from the second dispatch() invocation, thus a
dispatching to "resourceA" will happen.
What do you think? Are my assumptions correct?
Thanks
Violeta
Index: C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java
===================================================================
--- C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java (revision
1486660)
+++ C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java (working
copy)
@@ -216,8 +216,8 @@
}
};
+ this.request.getCoyoteRequest().action(ActionCode.ASYNC_DISPATCH,
null);
this.dispatch = run;
- this.request.getCoyoteRequest().action(ActionCode.ASYNC_DISPATCH,
null);
}
@Override
Re: AsyncContext.dispatch(path) invoked more than once
Posted by Mark Thomas <ma...@apache.org>.
On 04/06/2013 13:55, Violeta Georgieva wrote:
> 2013/5/31 Violeta Georgieva wrote:
>> Let's put this as plan B for now.
>>
>> I made a small change in the AsyncContextImpl.doInternalDispatch().
>>
>> Can you comment on the patch?
>>
>>
>> Index: C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java
>> ===================================================================
>> --- C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java
> (revision 1488110)
>> +++ C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java
> (working copy)
>> @@ -185,6 +185,10 @@
>> logDebug("dispatch ");
>> }
>> check();
>> + if (dispatch != null) {
>> + throw new IllegalStateException(
>> + sm.getString("asyncContextImpl.dispatchingStarted"));
>> + }
>> if (request.getAttribute(ASYNC_REQUEST_URI)==null) {
>> request.setAttribute(ASYNC_REQUEST_URI,
> request.getRequestURI());
>> request.setAttribute(ASYNC_CONTEXT_PATH,
> request.getContextPath());
>> @@ -347,7 +351,9 @@
>> logDebug("intDispatch");
>> }
>> try {
>> - dispatch.run();
>> + Runnable runnable = dispatch;
>> + dispatch = null;
>> + runnable.run();
>> if (!request.isAsync()) {
>> fireOnComplete();
>> }
>>
>>
>
> Can you comment?
Looks good to me.
> Any other suggestions?
Nope :)
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: AsyncContext.dispatch(path) invoked more than once
Posted by Violeta Georgieva <mi...@gmail.com>.
2013/5/31 Violeta Georgieva wrote:
>
> 2013/5/29 Violeta Georgieva wrote:
> >
> > 2013/5/28 Konstantin Kolinko wrote:
> > >
> > >
> > > I think that your patch is wrong.
> > >
> > > Looking at how ActionCode.ASYNC_DISPATCH is handled in different
> > > connector implementations in Tomcat 7, the code is like the following:
> > >
> > > if (asyncStateMachine.asyncDispatch()) {
> > >
((AprEndpoint)endpoint).processSocketAsync(this.socket,
> > > SocketStatus.OPEN);
> > > }
> > >
> > > In your example dispatching does not happen immediately as
> > > asyncDispatch() call returns "false" and asyncStateMachine state
> > > changes to AsyncState.MUST_DISPATCH. Thus your patch works.
> > >
> > > But, there is a case when the call returns "true" and dispatching
> > > happens immediately. Such dispatching will be broken.
> >
> > Thanks for pointing this.
> >
> > >
> > > BTW, I wonder if the following can be an alternative:
> > >
> > > if (this.dispatch != null) throw new IllegalStateException("..");
> >
> > Unfortunately this will break another scenario:
> >
> > Servlet1 does dispatch to Servlet2
> >
> > AsyncContext asyncContext = request.startAsync(request, response);
> > asyncContext .dispatch("/Servlet2");
> >
> > Servlet2 does dispatch to resourceA
> >
> > AsyncContext asyncContext = request.startAsync(request, response);
> > asyncContext .dispatch("/Servlet2"); <==== here we still do not
have invocation to AsyncContextImpl.recycle and AsyncContextImpl.dispatch
field is not null
> >
> >
> > So if we introduce a null check we will break double dispatch scenario.
> >
> > The solution might be to separate the check for state and the actual
action invocation.
>
> Let's put this as plan B for now.
>
> I made a small change in the AsyncContextImpl.doInternalDispatch().
>
> Can you comment on the patch?
>
>
> Index: C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java
> ===================================================================
> --- C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java
(revision 1488110)
> +++ C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java
(working copy)
> @@ -185,6 +185,10 @@
> logDebug("dispatch ");
> }
> check();
> + if (dispatch != null) {
> + throw new IllegalStateException(
> + sm.getString("asyncContextImpl.dispatchingStarted"));
> + }
> if (request.getAttribute(ASYNC_REQUEST_URI)==null) {
> request.setAttribute(ASYNC_REQUEST_URI,
request.getRequestURI());
> request.setAttribute(ASYNC_CONTEXT_PATH,
request.getContextPath());
> @@ -347,7 +351,9 @@
> logDebug("intDispatch");
> }
> try {
> - dispatch.run();
> + Runnable runnable = dispatch;
> + dispatch = null;
> + runnable.run();
> if (!request.isAsync()) {
> fireOnComplete();
> }
>
>
Can you comment?
Any other suggestions?
Thanks
>
>
> > Wdyt?
> >
> > Violeta
> >
> > >
> > > Best regards,
> > > Konstantin Kolinko
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > >
> >
>
Re: AsyncContext.dispatch(path) invoked more than once
Posted by Violeta Georgieva <mi...@gmail.com>.
2013/5/29 Violeta Georgieva wrote:
>
> 2013/5/28 Konstantin Kolinko wrote:
> >
> >
> > I think that your patch is wrong.
> >
> > Looking at how ActionCode.ASYNC_DISPATCH is handled in different
> > connector implementations in Tomcat 7, the code is like the following:
> >
> > if (asyncStateMachine.asyncDispatch()) {
> > ((AprEndpoint)endpoint).processSocketAsync(this.socket,
> > SocketStatus.OPEN);
> > }
> >
> > In your example dispatching does not happen immediately as
> > asyncDispatch() call returns "false" and asyncStateMachine state
> > changes to AsyncState.MUST_DISPATCH. Thus your patch works.
> >
> > But, there is a case when the call returns "true" and dispatching
> > happens immediately. Such dispatching will be broken.
>
> Thanks for pointing this.
>
> >
> > BTW, I wonder if the following can be an alternative:
> >
> > if (this.dispatch != null) throw new IllegalStateException("..");
>
> Unfortunately this will break another scenario:
>
> Servlet1 does dispatch to Servlet2
>
> AsyncContext asyncContext = request.startAsync(request, response);
> asyncContext .dispatch("/Servlet2");
>
> Servlet2 does dispatch to resourceA
>
> AsyncContext asyncContext = request.startAsync(request, response);
> asyncContext .dispatch("/Servlet2"); <==== here we still do not
have invocation to AsyncContextImpl.recycle and AsyncContextImpl.dispatch
field is not null
>
>
> So if we introduce a null check we will break double dispatch scenario.
>
> The solution might be to separate the check for state and the actual
action invocation.
Let's put this as plan B for now.
I made a small change in the AsyncContextImpl.doInternalDispatch().
Can you comment on the patch?
Index: C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java
===================================================================
--- C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java (revision
1488110)
+++ C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java (working
copy)
@@ -185,6 +185,10 @@
logDebug("dispatch ");
}
check();
+ if (dispatch != null) {
+ throw new IllegalStateException(
+ sm.getString("asyncContextImpl.dispatchingStarted"));
+ }
if (request.getAttribute(ASYNC_REQUEST_URI)==null) {
request.setAttribute(ASYNC_REQUEST_URI,
request.getRequestURI());
request.setAttribute(ASYNC_CONTEXT_PATH,
request.getContextPath());
@@ -347,7 +351,9 @@
logDebug("intDispatch");
}
try {
- dispatch.run();
+ Runnable runnable = dispatch;
+ dispatch = null;
+ runnable.run();
if (!request.isAsync()) {
fireOnComplete();
}
> Wdyt?
>
> Violeta
>
> >
> > Best regards,
> > Konstantin Kolinko
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
> >
>
Re: AsyncContext.dispatch(path) invoked more than once
Posted by Violeta Georgieva <mi...@gmail.com>.
2013/5/28 Konstantin Kolinko wrote:
>
>
> I think that your patch is wrong.
>
> Looking at how ActionCode.ASYNC_DISPATCH is handled in different
> connector implementations in Tomcat 7, the code is like the following:
>
> if (asyncStateMachine.asyncDispatch()) {
> ((AprEndpoint)endpoint).processSocketAsync(this.socket,
> SocketStatus.OPEN);
> }
>
> In your example dispatching does not happen immediately as
> asyncDispatch() call returns "false" and asyncStateMachine state
> changes to AsyncState.MUST_DISPATCH. Thus your patch works.
>
> But, there is a case when the call returns "true" and dispatching
> happens immediately. Such dispatching will be broken.
Thanks for pointing this.
>
> BTW, I wonder if the following can be an alternative:
>
> if (this.dispatch != null) throw new IllegalStateException("..");
Unfortunately this will break another scenario:
Servlet1 does dispatch to Servlet2
AsyncContext asyncContext = request.startAsync(request, response);
asyncContext .dispatch("/Servlet2");
Servlet2 does dispatch to resourceA
AsyncContext asyncContext = request.startAsync(request, response);
asyncContext .dispatch("/Servlet2"); <==== here we still do not
have invocation to AsyncContextImpl.recycle and AsyncContextImpl.dispatch
field is not null
So if we introduce a null check we will break double dispatch scenario.
The solution might be to separate the check for state and the actual action
invocation.
Wdyt?
Violeta
>
> Best regards,
> Konstantin Kolinko
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
Re: AsyncContext.dispatch(path) invoked more than once
Posted by Konstantin Kolinko <kn...@gmail.com>.
2013/5/28 Violeta Georgieva <mi...@gmail.com>:
> Hi,
>
> In the AsyncContext.dispatch(path) javadoc we have:
>
> "There can be at most one asynchronous dispatch operation per asynchronous
> cycle, which is started by a call to one of the
> ServletRequest.startAsync()<file:///C:/vily/my%20documents/servlet-3_0-final-javadoc/javax/servlet/ServletRequest.html#startAsync()>methods.
> Any attempt to perform an additional asynchronous dispatch
> operation within the same asynchronous cycle will result in an
> IllegalStateException. If startAsync is subsequently called on the
> dispatched request, then any of the dispatch or
> complete()<file:///C:/vily/my%20documents/servlet-3_0-final-javadoc/javax/servlet/AsyncContext.html#complete()>methods
> may be called."
>
> If we have the following scenario:
>
> AsyncContext asyncContext = request.startAsync(request, response);
> asyncContext .dispatch("/resourceA");
> asyncContext .dispatch("/resourceB");
>
> I would assume that the second dispatch will throw ISE and a dispatching to
> the "resourceA" will happen.
>
> The current implementation (tomcat7/8) throws ISE but the dispatching is to
> the "resourceB".
>
> I prepared a change (below) that moves the check for ISE a little bit
> earlier. o.a.catalina.core.AsyncContextImpl.dispatch field is not
> overridden with information from the second dispatch() invocation, thus a
> dispatching to "resourceA" will happen.
>
> What do you think? Are my assumptions correct?
>
> Thanks
> Violeta
>
>
> Index: C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java
> ===================================================================
> --- C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java (revision
> 1486660)
> +++ C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java (working
> copy)
> @@ -216,8 +216,8 @@
> }
> };
>
> + this.request.getCoyoteRequest().action(ActionCode.ASYNC_DISPATCH,
> null);
> this.dispatch = run;
> - this.request.getCoyoteRequest().action(ActionCode.ASYNC_DISPATCH,
> null);
> }
>
> @Override
I think that your patch is wrong.
Looking at how ActionCode.ASYNC_DISPATCH is handled in different
connector implementations in Tomcat 7, the code is like the following:
if (asyncStateMachine.asyncDispatch()) {
((AprEndpoint)endpoint).processSocketAsync(this.socket,
SocketStatus.OPEN);
}
In your example dispatching does not happen immediately as
asyncDispatch() call returns "false" and asyncStateMachine state
changes to AsyncState.MUST_DISPATCH. Thus your patch works.
But, there is a case when the call returns "true" and dispatching
happens immediately. Such dispatching will be broken.
BTW, I wonder if the following can be an alternative:
if (this.dispatch != null) throw new IllegalStateException("..");
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: AsyncContext.dispatch(path) invoked more than once
Posted by Mark Thomas <ma...@apache.org>.
On 28/05/2013 19:51, Violeta Georgieva wrote:
> Hi,
>
> In the AsyncContext.dispatch(path) javadoc we have:
>
> "There can be at most one asynchronous dispatch operation per asynchronous
> cycle, which is started by a call to one of the
> ServletRequest.startAsync()<file:///C:/vily/my%20documents/servlet-3_0-final-javadoc/javax/servlet/ServletRequest.html#startAsync()>methods.
> Any attempt to perform an additional asynchronous dispatch
> operation within the same asynchronous cycle will result in an
> IllegalStateException. If startAsync is subsequently called on the
> dispatched request, then any of the dispatch or
> complete()<file:///C:/vily/my%20documents/servlet-3_0-final-javadoc/javax/servlet/AsyncContext.html#complete()>methods
> may be called."
>
> If we have the following scenario:
>
> AsyncContext asyncContext = request.startAsync(request, response);
> asyncContext .dispatch("/resourceA");
> asyncContext .dispatch("/resourceB");
>
> I would assume that the second dispatch will throw ISE and a dispatching to
> the "resourceA" will happen.
>
> The current implementation (tomcat7/8) throws ISE but the dispatching is to
> the "resourceB".
>
> I prepared a change (below) that moves the check for ISE a little bit
> earlier. o.a.catalina.core.AsyncContextImpl.dispatch field is not
> overridden with information from the second dispatch() invocation, thus a
> dispatching to "resourceA" will happen.
>
> What do you think? Are my assumptions correct?
+1
Mark
>
> Thanks
> Violeta
>
>
> Index: C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java
> ===================================================================
> --- C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java (revision
> 1486660)
> +++ C:/tc8.0.x/java/org/apache/catalina/core/AsyncContextImpl.java (working
> copy)
> @@ -216,8 +216,8 @@
> }
> };
>
> + this.request.getCoyoteRequest().action(ActionCode.ASYNC_DISPATCH,
> null);
> this.dispatch = run;
> - this.request.getCoyoteRequest().action(ActionCode.ASYNC_DISPATCH,
> null);
> }
>
> @Override
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org