Issue #4462 - Prevent jetty 10 WebSocket close deadlocks

[lachlan-roberts]

Closes #4462

[joakime]

Something is going on with this build / test.
It's taking far too long (currently at 1 hour 47 minutes and counting)

[joakime]

Last change breaks 8 test cases.

[lachlan-roberts]

I changed it so we no longer fail the callback with a ISE if a frame is sent and we are closed, now it is a ClosedChannelException which seems more correct. The javax API close throws IOException so now if you do a second close you will get the ClosedChannelException.

[sbordet]

@lachlan-roberts I would like to see, if it's not there yet, a test that cements the behavior of core for closes: that for 2 concurrent closes, the loser gets an exception thrown.

Similarly, I would like to see a test that proves that it's still possible to write from onWebSocketClose() if the close is normal (if the test is not there already).

Would be great if you can test concurrency of flushBatch() and close() as well and again cement the expected behavior in a test.

[lachlan-roberts]

@simone I introduced the extra tests you wanted and addressed the review points.

Similarly, I would like to see a test that proves that it's still possible to write from onWebSocketClose() if the close is normal (if the test is not there already).

It is never possible to write from onWebSocketClose(), even when the close is normal, this is notified when the close handshake is completely done and the connection is closed and not when you just received a close frame.

[sbordet]

It is never possible to write from onWebSocketClose(), even when the close is normal, this is notified when the close handshake is completely done and the connection is closed and not when you just received a close frame.

That is so wrong.
Applications must be able to send messages from onClose events, things like "last processed item" and such. Otherwise there is no point in notifying the event, if applications cannot do anything from the callback apart logging.

[gregw]

I'm wondering if we should repeat the pattern (mistake?) of HTTP here. With HTTP we have a separately configurable blocking timeout, that only if not set do we default to idleTimeout. This way we can handle (if need be) the timeouts for sending a huge frame that might take longer than the idle timeout, even though it is never actually idle.

However, in HTTP, we have deprecated the blocking timeout in favour of the data rate mechanisms.... Which we don't really have here (but are probably good ideas for @ extensions?), but it may still be good to have to turn off the blocking timeout if we have confidence in our callback mechanism.

@sbordet - thoughts? I'm only making this a comment as I don't think we should hold this PR up while we ponder this.

[gregw]

LGTM - still lots of questions about this, but I think we should merge and resolve in other PRs