On 12/04/2013 15:18, Rémy Maucherat wrote:
> 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.
This is what I am doing now and things are a little messy. I keep
finding places where I now have to protect against exceptions of one
form or another because the socket has been closed on the other thread.
I probably need to go through and do a little refactoring to clean this up.
OK I am biased, but I think this is the behaviour that developers would
expect from the HTTP Upgrade API.
I still need to do the testing to ensure that a limit of zero or one
read threads and zero or one write threads is sufficient to implement
the WebSocket API. I hope to do this over the coming days.
> 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).
I think the problem is a little more difficult than that.
WebSocket requires a call-back (to user code) once the async send has
completed. Consider the following:
- Large async write
(Handled inside container - no user code)
- Container thread reading data in user code triggers a blocking write
(async above still on-going)
- Large sync write
(Auto-blocking starts, container thread blocked)
- Async write completes triggering a call back on a container thread to
user code
At this point you have two container threads in user code. One is
blocked but the need for two threads triggers essentially the same
complications as my current solution (all two threads, one for read,
one for write) and adds the complexity of auto-blocking.
One way around this would be to delay the call-back from the async write
until the sync write has completed. That solves the problem of multiple
threads in user code but doesn't remove the requirement for multiple
threads in container code.
> 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).]
My WebSocket implementation is written on top of the HTTP Upgrade
mechanism so all it has to work with are Servlet[Input|Output]Stream. If
could work-around these issue by accessing the container directly but
the whole point was to make the WebSocket implementation container neutral.
> 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
My preference is for allowing/requiring concurrent read/write but with a
limit of no more than one thread in each. It is partly because this is
what I have now, partly because whenever I think about a solution I end
up with needing multiple threads (as a minimum in the container) so lets
embrace the idea and partly because it is what I think a developer expects.
Next is auto-blocking. My main reason against it is the unknown factor.
I can see how it should work but I'm concerned about what gotchas may
emerge in the detail of an implementation.
I don't see how WebConnection.start(Runnable) helps. If user code blocks
in one container thread (to simulate a blocking write) a second
container thread has to be able to access the user code to single that
the block should finish.
Mark