Issue #5368 - ensure onMessage exits before next frame is read

[lachlan-roberts]

Issue #5368
When reading a websocket message with a Reader or InputStream ensure that the onMessage() method has exited before resuming to read and parse websocket frames. Otherwise a call to the MessageInputStream.read() or MessageInputStream.close() could parse the next websocket frame which could trigger a second nested call to onMessage() in the same thread.

[joakime]

Relying on exit/return from the onMessage() only with this PR?
We need to tread carefully here. Past experience shows this can be problematic.

Example:

We get a TEXT Message as series of frames.
It contains a JSON message, along with more text after the JSON message (it could be whitespace, it could be garbage, it doesn't matter what it is for this example, just that it exists, and is non-trivial in number of frames/size).
The implementation issues a onMessage(InputStream) and the application uses an old school JSON library that just reads/parses up to the close of the JSON message.
The application thinks it's done and exits the onMessage(InputStream)
There's now more unread / unparsed frames on that message, but no application reading them.
Is this a discard moment? I don't know.
Quietly discarding data seems like a bad approach.
We need to be NOISY in logs if this occurs (eg: onMessage is exited, but the InputStream isn't at EOF, and the InputStream gets more data, we should log at WARN level each and every frame the application is not reading. Let them implement their onMessage properly to get rid of the noise)
Or we should be extra-harsh in this scenario and terminate the connection with an onError() indicating that the onMessage() triggered an EarlyEof scenario.

[lachlan-roberts]

@joakime we do need to pay attention to the exit for the onMessage(), that is the only way to ensure we don't call it twice. But we are not relying on them fully consuming all the data from the websocket message. Currently we will just discard any remaining content and any new frames left of the websocket message if they exit early or close the InputStream. So if they decide they don't want to process the rest of the message for whatever reason they can just return or close the InputStream.

I'm not opposed to putting a warning if we end up discarding content, but I feel like it would be excessive to warn once per frame, once per message seems fine. But I don't think its a good idea to be extra-harsh and terminate the connection. This could be a breaking change for people relying on the discard behavior.

[joakime]

I have questions / concerns...

[joakime]

Once the CI build is green, go ahead and merge.