On 9 December 2014 at 15:00, Mark Thomas <markt_at_apache.org> wrote:
[snip]
> Again, if the call to startAsync was moved to Z we have this problem
> right now.
>
>
I'm not sure why moving the startAsync to Z changes anything?
But regardless, we do already have this problem now. It is very much
undefined what request methods can be called from an asynchronous thread
and what values they should return. The path methods are just one
example. Another is can an async thread call getParameter, which may
consume input? What if the input has been put into async IO mode? what if
it has been partially consumed already? Is that threadsafe?
My point is that we have got away with this vague spec so far because we
basically say that the request/response objects are not thread safe and if
you want thread safety for them then use them only from a container managed
thread. We have made an exception to that for the input/output streams.
If we allow async threads to call doFilter, then we are essentially saying
that all APIs can be called by an async thread. So we have a lot of work
do to to clarify the multi-threaded behaviour of the whole API which has
previously just been declared thread unsafe.
> > 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.
>
I'm trying to say that the spec requires that any request/response wrappers
passed into RequestDispatcher.forward(...) are preserved unwrapped in the
subsequent calls to service, so that filters/servlets can downcast the
request/response to the application wrapped classes. This prevents the
container from wrapping for forward/include semantics and thus prevents
wrapping to be used as a tool to control the different views of the same
request/response object.
> > 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".
>
>
Sorry for dramatic language. But I do think we have a principal
established that it is only container managed threads and only one of them
per request that is within the servlet container. Allowing other threads
to call request dispatchers and filter chains does change something
fundamental.
> The restriction on a single container thread still applies.
I don't understand how that can be enforced if we allow arbitrary threads
to call forward and doFilter?
> 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 thought that was implicit in the specification as per the crappy state
diagram (written by me) in 2.3.3.3. That state machine is an example of
how we have tried to ensure only a single thread is ever dispatched for a
request/response.
> 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.
>
I think the app/container can access the request/response normally until
such time as the dispatched thread returns to the container.
For async IO we have directly supported this by delaying onWritePossible
and onDataAvailable callbacks until the original thread returns. For
normal async, we support this by delaying the action of complete() and
dispatch() until the return.
> 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.
>
It could be argued that way, but I would argue that it was a clarification
of a restriction that always existed.
The expectation has always been that a servlet will be invoked within the
scope of the filters that are mapped to it. Thus exceptions thrown by the
servlet can be caught by the filter, thread locals set by the filter can be
seen by the servlet, wrappers applied by the filter are seen by the
servlet, etc.
If we allow a filter in the filter chain to use another thread to continue
the chain, then most of those assumptions are lost.
I totally accept your devils advocate point of view here. It is a very
good question to ask as to why we don't allow this. Yet I think our
response to this has to be to tighten the specification rather than
liberalise it. Then we need to look at the specific use-case that has
raised this and see how we can best support that use-case without breaking
some fundamental assumptions in the servlet design.
cheers.
--
Greg Wilkins <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.