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....

[gregw]

To decide the best course to take on this one, I think we need to be very explicit about the responsibilities of a Demanding listener (auto-demand == false).

With auto-demand, the listener never needs to call demand().

The simple flip side of this would be that without auto-demand, then listener always needs to call demand(). However, we cannot be so simplistic, because we do call demand() for the listener in some circumstance. Specifically:

1. if a message handler is registered for a frame type that can be continued, then we will demand on non-fin frames so that continuations will be demanded.
2. if a message handler is not registered for a frame type, then we call demand

It is case 2) above that is the problematic one, as if the listener implements onWebSocketFrame and wants to handle demand itself, then it cannot because demand is done automatically for it because there is no message handler for that frame type.

So I think the first step of the solution is to modify the cases in which we will call demand on a non-auto listener to be:

1. if a message handler is registered for a frame type that can be continued, then we will demand on non-fin frames so that continuations will be demanded.
2. if a message handler is not registered for a frame type AND there is no onWebSocketFrame handler, then we call demand.

This can be paraphrased as we will call demand only when the event of the frames arrival is not notified to the listener (either waiting for a FIN or because there was no handler).

I think this is a good change and should be our starting point to allowing implementations to be in onWebSocketFrame. That will be fine if a listener only implements onWebSocketFrame, but it is a little complex if they want to mix modes like:

public class MyWebSocketListener implements Session.Listener
{
    private Session _session;
    
    @Override
    public void onWebSocketOpen(Session session)
    {
        _session = session;
    }

    @Override
    public void onWebSocketFrame(Frame frame, Callback callback)
    {
        System.err.println(frame);
        if (frame.getOpCode() != Frame.Type.TEXT.getOpCode())
            _session.demand();
    }

    @Override
    public void onWebSocketText(String message)
    {
        System.err.println(message);
        _session.demand();
    }
}

The problem here, is that this doesn't work due to non-fin continuation frames for a text, which will be demanded both by the onWebSocketFrame and the onWebSocketText method. To deal with this, the listener would need to track the sequence of frames, so they can work out if it will be handled or not. This can perhaps be solved with an extra API that allows us to know if a continuation frame is text or binary. Something like Frame.getEffectiveOpCode(), which would return TEXT if it is a continuation frame after a Text frame. This could be used as follows:

public class MyWebSocketListener implements Session.Listener
{
    private Session _session;
    
    @Override
    public void onWebSocketOpen(Session session)
    {
        _session = session;
    }

    @Override
    public void onWebSocketFrame(Frame frame, Callback callback)
    {
        System.err.println(frame);
        if (frame.getEffectiveOpCode() != Frame.Type.TEXT.getOpCode())
            _session.demand();
    }

    @Override
    public void onWebSocketText(String message)
    {
        System.err.println(message);
        _session.demand();
    }
}

Finally, there is the issue of existing applications that are overriding onWebSocketFrame and a method like onWebSocketText in non-auto mode. Probably very VERY few such applications, because it is difficult to write such apps with the current application. But if we REALLY wanted to be nice to them, we could provide a legacy mode, where the former behaviour was supported.

[gregw]

Looking good... but some questions/niggles.

But most importantly, it would be good to see an example and/or test actually using getEffectiveOpCode()

[gregw]

javadoc needed as this looks to be a combination of a deep and shallow copy???

[sbordet]

Is this necessary? Only used in tests.

[lachlan-roberts]

One of the tests was saving the Frame past the onWebSocketFrame method, but the frames payload is released after this. So if that was testing something we want to support, then we should allow them to copy the frame like we do in websocket-core. Otherwise I will have to change the test to not save the Frame.

[sbordet]

I'm for giving retain semantic to a Frame, which is a concept that we have elsewhere.

Adding copy() seems to introduce another, different, mechanism for applications, and if we later decide to go with retain, we will have to maintain two mechanisms.

I think the tests should be changed, if it wants to retain the payload.
If this behavior documented, that the frame must be consumed in the context of a WebSocket event method?

[lachlan-roberts]

No point having a retain as the buffer is already retained until the callback is completed.

I changed the test and removed the copy/wrapper.

[sbordet]

I would prefer to avoid forcing applications that implement onWebSocketFrame() to have to send PONGs.

I prefer the solution where either you have onWebSocketFrame() and nothing else, or no onWebSocketFrame() with other methods.

We already enforce similar rules at deployment for text and partial text, or binary and partial binary, exactly to avoid processing the same frame twice, and I think this is no exception.

[sbordet]

Is this necessary? Only used in tests.

[gregw]

Other than a thought bubble, this looks good

[sbordet]

I'm for giving retain semantic to a Frame, which is a concept that we have elsewhere.

Adding copy() seems to introduce another, different, mechanism for applications, and if we later decide to go with retain, we will have to maintain two mechanisms.

I think the tests should be changed, if it wants to retain the payload.
If this behavior documented, that the frame must be consumed in the context of a WebSocket event method?

addresed