On 04/12/2013 02:55 PM, Mark Thomas wrote:
> If the restriction is one container thread per request then there is a
> problem. The container thread that triggers the blocking write has to
> block until the write completes and a second container thread (for the
> same request) has to trigger the onWritePossible() so you have two
> container threads working with the same request. The restrictions as
> you described them do not allow this.
>
> Going in to a bit more detail, the container thread that triggers a
> blocking write will always have been triggered by data being read from
> the client (i.e. it is using the ReadListener). The second thread is
> using the WriteListener so we never have more than two threads with
> one thread working with the ReadListener and one with the WriteListener.
>
> If the restriction in 3.7 / 5.3 is no more than one container thread
> to use a [Read|Write]Listener at a time (i.e. two concurrent container
> threads, one calling the ReadListener, one calling the WriteListener
> is permitted) then I think (I'd need to do some testing to check) all
> is good.
I think this breaks the Servlet threading model, where the rule is that
one container thread is running user code at one point in time. If this
is allowed, thread safety issues can now occur everywhere and all
structures need to be thread safe. OTOH, this could be relaxed in
upgraded mode since this is no longer Servlets (in theory), but this
still possibly problematic.
So the websocket use case is a problem, since it clearly should be async
read/write, but allows mixing things, for example where as a response to
onMessage, you start using, say, a blocking|**||**| getSendWriter to
send a large amount of data. This is never going to work really well,
since the Servlet API removed autoblocking (in container threads) which
was intended at that use case (you want to send some random amount of
data easily when processing a read notification).
Without the clarification, I would say the websocket library has to be
able to access the container thread pool at some point to tell it to run
its code (to cover the fact that onMessage can do blocking operations).
The AsyncContext.start(Runnable) functionality could have been used
there, but I don't think it is available. [Note: I never actually
understood if the actual execution of it was supposed to be fully
asynchronous (the container takes a new thread from its pool to execute
it directly and does not wait for it to complete). This is not what I am
doing in my impl (since I understood it as being equivalent to running
the Servlet code).]
So the possible solutions (in my personal order of preference) are:
- Adding autoblocking (it is vaguely mentioned as being "illegal" right
now; if this is not mentioned anymore, the API semantics apply, and they
should block): it is easy, sneaky and solves the problem, but the
implementation changes are also significant
- Allowing (actually: requiring) concurrent read/write events: it sounds
simple but it leads to more issues, and it should really only be done
once upgraded, but it could still lead to implementation problems (needs
investigation)
- Add a WebConnection.start(Runnable): it makes the API clunky and
people would wonder why it is there
Indeed, extending the time for "final" review would have been needed, it
is only really possible to find issues like that when implementing.
Rémy