websocket icon indicating copy to clipboard operation
websocket copied to clipboard

Clarify concurrency of Basic and Async messaging

Open markt-asf opened this issue 6 years ago • 2 comments

The Javadoc for RemoteEndpoint.Basic has the following text

If the websocket connection underlying this RemoteEndpoint is busy sending a message when a call is made to send another one, for example if two threads attempt to call a send method concurrently, or if a developer attempts to send a new message while in the middle of sending an existing one, the send method called while the connection is already busy may throw an java.lang.IllegalStateException.

This was originally on RemoteEndpoint but after the refactoring to Basic and Async was performed the text only appeared on Basic.

There are several issues.

  1. The use of MAY By defining that an exception "may" be thrown in this case, the user of this API has to code to handle this case since an exception "may" be thrown. This results in unnecessary work if the implementation opts not to throw an exception. I suggest that this instance of "may" is changed to either "must" or "must not". I have no strong preference for either.

  2. Inconsistent concurrency requirements. It is not clear if this "no concurrent send message calls" requirement applies to Async or not. This should be clarified. Note that the TCK suggests that this requirement does not apply to Async. I suggest that Basic and Async follow the same rule and either both allow concurrent calls or both do not.

If concurrent calls to send message methods are allowed then that raises the question of how to handle them since they are not permitted on the wire (ignoring the multiplexing extension). Do subsequent messages get buffered until the first completes? If yes, how is that buffer managed? What prevents a OOME if messages are sent slower than the application writes them? Where is the back pressure? Alternatively - and perhaps simpler to implement - do subsequent calls block until the previous call completes? In this approach are the blocked threads placed in a FIFO queue?

markt-asf avatar Feb 05 '19 19:02 markt-asf

  1. The use of MAY By defining that an exception "may" be thrown in this case, the user of this API has to code to handle this case since an exception "may" be thrown. This results in unnecessary work if the implementation opts not to throw an exception. I suggest that this instance of "may" is changed to either "must" or "must not". I have no strong preference for either.

Using MUST means there's no buffering/batching of messages. Using MUST NOT means a possibility of infinite buffering/batching.

Neither option sounds good to me.

Don't forget the wrinkle that RemoteEndpoint batching has on this discussion as well. "batching" unfortunately was ill defined by the original spec and has come to have multiple meanings in the various implementations.

joakime avatar Feb 05 '19 19:02 joakime

I hadn't thought of it from that perspective. The current wording means that back-pressure is provided by throwing an IllegalStateException for Basic which seems wrong to me. It also suggests infinite buffering for Async which also does not sound good. I'm not set on any particular solution but I do think we need some clarification here. More thought required...

markt-asf avatar Feb 06 '19 11:02 markt-asf