On 09/12/2014 13:31, Greg Wilkins wrote:
>
> I'm +1000000 on Stuarts concern about multiple threads dispatched into
> the filter/servlet chain at the same time.
>
> Originally the async servlet design was done not for async IO. It's
> design was primarily motivated by startAsync() to
> AsyncContext.dispatch() style handling, so that all request handling and
> response generation was still to be done within a container thread
> dispatched to the filter/servlet using blocking APIs.
>
> This design was modified to support the startAsync() to
> AsyncContext.complete() style of usage where the asynchronous thread
> writes the response and then calls complete. The problem is that we
> never really well defined what methods on the request/response can be
> safely called by such asynchronous threads.
>
> We have now added async IO as a further extension this start->complete
> style, but still have not well defined what methods can be called. I
> think if we add asynchronous concurrent calls to the filter/servlet then
> we will have a real mess.
>
> Consider the following example:
>
> * Context A has Servlet X
> * Context B has Servlets Y & Z each with a Filter P and Q mapped in
> front of them
> * Context A Servlet X does a cross context forward to Context B Servlet Y
> * Context B Filter P does does a dispatch forward to Servlet Z
> * Context B Filter Q calls startAsync, starts a new thread which calls
> chain.doFilter, the returns.
>
> So now we have 2 threads in the servlet container handling the same
> request. One of them is trying to call Context B Servlet Z and the
> other is returning back via the stack of invocations.
If the call to startAsync was moved to Z the above is all possible
ignoring the current discussion regarding Filters and 6.2.3. Therefore
if there are problems here that is a separate issue that needs to be
resolved regardless of the outcome of this discussion.
> In this situation, what are the return values from methods like
> getContextPath, getSerlvetPath and getPathInfo? as the original
> thread returns back via its call stack, the return values for these
> methods will change from B/Z to B/Y and finally back to A/X. The async
> thread calling doFilter can reasonably expect these methods to return
> B/Z for the duration of the call.
Again, if the call to startAsync was moved to Z we have this problem
right now.
> Note that we can't solve this problem by request wrapping as the spec
> requires == for the request objects passed into forward with that used
> in the next dispatch.
Sorry, I can't parse that sentence. I suspect a word or two are missing.
> Are we going to have to implement these methods with ThreadLocals?
>
> The Request/Response and RequestDispatcher classes were just not
> designed for multithreaded access. We have gone to considerable
> lengths to ensure only a single thread is ever dispatched to the servlet
> container (eg onWritePossible and onReadPossible are delayed until the
> original thread exits the container). So I don't think we can
> realistically throw this all away and suddenly allow arbitrary threads
> to be simultaneously calling the filter/servlet
Nothing that is being discussed requires "throwing this all away".
The restriction on a single container thread still applies. As an aside,
Tomcat has recently added delaying the processing of complete() and
dispatch() from a non-container thread until the container thread where
AsyncContext.start() was called as completed to avoid various
thread-safety issues.
I think there is an undocumented (which we should fix) expectation that,
after startAsync() has been called, neither the app nor the container
accesses the request or the response.
There is currently no restriction on the number of non-container threads
but thread-safety is an application concern. So no change there either.
The restriction in 6.2.3 was only added in Servlet 3.1 so in Servlet 3.0
it could be argued that this is permitted and 6.2.3 was a backward
compatibility breaking change in Servlet 3.1.
Mark
> On 9 December 2014 at 00:13, Stuart Douglas <stuart.w.douglas_at_gmail.com
> <mailto:stuart.w.douglas_at_gmail.com>> wrote:
>
>
>
> Edward Burns wrote:
>
> On Tue, 09 Dec 2014 08:04:50 +1100, Stuart
> Douglas<sdouglas_at_redhat.com
> <mailto:sdouglas_at_redhat.com>> said:
>
>
> MT> Section 6.2.3 includes the following text:
> MT> <quote>
> MT> A Filter and the target servlet or resource at the end of
> the filter
> MT> chain must execute in the same invocation thread.
> MT> </quote>
> MT>
> MT> As a result of a bug raised against Apache Tomcat [1] I am
> seeking
> MT> clarification of the meaning of that sentence.
> MT>
> MT> As written, the meaning seems unambiguous. However, does it
> apply when
> MT> the request is in asynchronous mode? To put it another way
> is the
> MT> following legal?
> MT> - filter calls ServletRequest.startAsync()
> MT> - filter calls AsyncContext.start(Runnable)
> MT> - That Runnable called FilterChain.doFilter()
>
> SD> Another potential problem with doing this is that
> AsyncContext.start()
> SD> does not wait for the current call stack to return to the
> container (at
> SD> least it is not required to). This means that you will have
> two threads
> SD> potentially modifying the response, which is not thread safe.
>
> SD> We could modify the behaviour of AsyncContext.start() to
> wait for the
> SD> request to return to the container before actually running
> the task.
>
> SD> I think that this is actually a real thread safety problem
> in the
> SD> Servlet API, any code that uses AsyncContext.start() and
> then wants to
> SD> modify the response cannot be sure the original call stack
> has returned,
> SD> and could potentially have thread safety issues.
>
> Being new to this spec, I went looking for some catch-all text that
> calls out these obvious thread safety issues. Indeed, there is
> section
> 2.3.3.4 Thread Safety:
>
> Spec> Other than the startAsync and complete methods,
> implementations of
> Spec> the request and response objects are not guaranteed to be
> thread
> Spec> safe. This means that they should either only be used
> within the
> Spec> scope of the request handling thread or the application must
> Spec> ensure that access to the request and response objects
> are thread
> Spec> safe.
>
> Spec> If a thread created by the application uses the
> container-managed
> Spec> objects, such as the request or response object, those
> objects
> Spec> must be accessed only within the object's life cycle as
> Spec> defined in sections Section 3.12, "Lifetime of the Request
> Spec> Object" on page 3-31and Section 5.7, "Lifetime of the
> Spec> Response Object" on page 5-50 respectively. Be aware that
> Spec> other than the startAsync, and complete methods, the
> request and
> Spec> response objects are not thread safe. If those objects were
> Spec> accessed in the multiple threads, the access should be
> Spec> synchronized or be done through a wrapper to add the thread
> Spec> safety, for instance, synchronizing the call of the
> methods to
> Spec> access the request attribute, or using a local output
> stream for
> Spec> the response object within a thread.
>
> Need we say more?
>
>
> I guess my main concern here is that there is not really any way to
> hand off to another thread so that only one thread can be using the
> request/response at a time.
>
> Basically I am talking about some kind of AsyncContext.delayStart()
> construct that will not dispatch the task until the current call
> stack has returned to the container (or just change the semantics of
> start() to match). This will make sure that only one thread is using
> the request and response, without needing to resort to
> synchronization or other thread safety constructs.
>
> Stuart
>
>
>
>
>
> Ed
>
>
>
>
> --
> Greg Wilkins <gregw_at_intalio.com <mailto:gregw_at_intalio.com>> @ Webtide
> - /an Intalio subsidiary/
> http://eclipse.org/jetty HTTP, SPDY, Websocket server and client that scales
> http://www.webtide.com advice and support for jetty and cometd.