Issue #11307 - Explicit demand control in WebSocket endpoints with only onWebSocketFrame

[lachlan-roberts]

Issue #11307

Support endpoints in the Jetty WebSocket API to be use only a onWebSocketFrame method. They should demand in the onWebSocketFrame if they don't have another handler registered for this frame type.

There are some surprising changes of behavior needed to implement this which will likely surprise users if they are using onWebSocketFrame.

• For PING frames, if there is only onWebSocketFrame and no onWebSocketPing method defined, then we now expect the user to handle PING frames in their onWebSocketFrame, so we will no longer send an automatic frame in this case.
• If they had a onWebSocketText and no onWebSocketBinary and also a onWebSocketFrame, then they would now be expected to demand for BINARY/CONTINUATION opCodes in the onWebSocketFrame method.

[sbordet]

Solution 1. Keep current behavior.

The problem is that by only having onWebSocketFrame(), applications won't be able to explicitly demand: upon return of the method, there is automatic internal demand (because the specific frame handling methods are missing).
This means that onWebSocketFrame() cannot do asynchronous operations, such as writing the frame, and only when the write is complete then demand: it would have to block on the write.
We could document this but it's an awkward behavior.

Solution 2. Deprecate/Remove onWebSocketFrame()

Would break applications that override onWebSocketFrame(), but would give consistent behavior, since there would be no ambiguity: each frame is delivered only once to its specific method (ping, pong, text, binary, close), not twice (once to onWebSocketFrame(), and then to another method specific to the frame type).
Specific frame handling methods already treat demand correctly.

Solution 3. Try to fix onWebSocketFrame() (this PR)

Applications that only override onWebSocketFrame() would now have explicit control on demand.
However, applications that override also another frame handling method, would possibly have a problem.
This is because old implementations of onWebSocketFrame() won't work anymore (they are not demanding, but they should, so they would need to be modified), and also the demand in onWebSocketFrame() must be filtered: demand for all frames except the frame type for which there is a frame handling method.
For example, onWebSocketFrame() + onWebSocketText would require onWebSocketFrame() to have logic similar to this:

void onWebSocketFrame(Frame frame, Callback callback) {
  log.info(frame);
  if (frame.getType() != Frame.Type.TEXT)
    session.demand();
}

This is further complicated by the fact that CONTINUATION frames don't carry whether they are TEXT or BINARY, so the application would need to store this additional information to decide whether to demand or not (would not demand if it is a TEXT CONTINUATION, but would if it is a BINARY CONTINUATION for which there is no specific frame handling method).

---

I prefer Solution 2.
We could fail deployment if we detect the @OnWebSocketFrame annotation, or the onWebSocketFrame() method (and deprecate both), but the behavior would be consistent and simpler for applications.

@gregw @lachlan-roberts @lorban thoughts?

[lorban]

I definitely don't like solution 3 as it comes with too many surprising behaviors.

Now, between solutions 1 and 2, I'm not sure which ones is best. It feels like we would like a new mechanism incompatible with the existing one, but still keep the existing one for backward compatibility.

Do we want to maintain both? In that case, would something similar to the client's onContentSource mechanism where a single mechanism that totally controls the demand loop is implemented, with easier APIs (like an auto-demanding one) on top work here?

[sbordet]

@lorban the "single mechanism" would be onWebSocketFrame(), and you'd do everything from there, but this is too low-level for applications.
We don't want applications to handle CONTINUATION frames, coalesce chunks to create a WebSocket message, etc. as they would have too much "implementation" work to do.

The idea with the "high" level WebSocket APIs is that applications need only to override what they are interested in (for example, only text messages), and only write application logic.

[lachlan-roberts]

I agree that solution 3 is too tricky for someone to write their application correctly.
They should either deal in Frames or deal in the other Session.Listener notifications, doing both is trouble.
And we now have websocket-core if they want to use Frames directly.

Didn't we add some rule that to remove a method we need to deprecate it for 6months + x dot releases. So if we follow that rule we can never remove it for 12.1.x anyway. So we could just deprecate in 12.1.0 and remove in the next major release?

Another alternative would be to not deprecate it, but enforce that if they use the onWebSocketFrame, they cannot have any of the onPing/onPong/onText/onBinary notifications or the endpoint will fail to deploy.

[sbordet]

About failing the deployment, I think we already forbid the case where an application overrides both onWebSocketPartialText and onWebSocketText, because we do not want to notify twice.

The case of onWebSocketFrame + some other method is similar, and we should fail the deployment.

Solution 4. Restrict deployment
Allow only onWebSocketFrame, and fail deployment if some other method is present.

---

I prefer Solution 2 because Solution 4 would work at deployment, but then likely break the application.
In case of Solution 4, application writers would have to modify the code to add the demand calls, otherwise it may not work.
At least with Solution 2 you know you have to change things because it does not deploy.

[lachlan-roberts]

Well I'm assuming solution 2 is really to deprecate it because of our new removal policy, so very similar to solution 1.
Then we can remove it in the next major release 12.2.0 or 13.0.0.

[gregw]

I'm currently "none of the above". Let me get my head into this a bit more.... stand by....