Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify concurrency of Basic and Async messaging #288

Open
markt-asf opened this issue Feb 5, 2019 · 2 comments
Open

Clarify concurrency of Basic and Async messaging #288

markt-asf opened this issue Feb 5, 2019 · 2 comments
Labels
API (Both) Impacts the client and server API enhancement Adding a new feature or improving an existing one
Milestone

Comments

@markt-asf
Copy link
Contributor

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?

@joakime
Copy link
Contributor

joakime commented Feb 5, 2019

  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.

@markt-asf
Copy link
Contributor Author

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 markt-asf added API (Both) Impacts the client and server API enhancement Adding a new feature or improving an existing one Jakarta EE 10 labels Apr 16, 2020
@markt-asf markt-asf added this to the backlog milestone May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API (Both) Impacts the client and server API enhancement Adding a new feature or improving an existing one
Projects
None yet
Development

No branches or pull requests

2 participants