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

Fixes #9552 - Jetty 12 - Rewrite of the Jetty WebSocket APIs. #9652

Merged
merged 26 commits into from
May 2, 2023

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Apr 14, 2023

  • Removed unnecessary classes, among which BatchMode, CloseStatus, etc.
  • Coalesced all listener interfaces into Session.Listener.
  • Moved RemoteEndpoint functionality to Session.
  • Renamed WebSocketPolicy to Configurable.
  • Renamed WriteCallback to just Callback, as it is now also used for some listener methods.

* Removed unnecessary classes, among which BatchMode, CloseStatus, etc.
* Coalesced all listener interfaces into Session.Listener.
* Moved RemoteEndpoint functionality to Session.
* Renamed WebSocketPolicy to Configurable.
* Renamed WriteCallback to just Callback, as it is now also used for some listener methods.

Signed-off-by: Simone Bordet <[email protected]>
@@ -193,5 +170,157 @@ default WebSocketPolicy getPolicy()
*
* @return the suspend token suitable for resuming the reading of data on the connection.
*/
// TODO: remove and replace with demand()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely remove. This API is always a race!

sbordet added 2 commits April 15, 2023 12:12
…subset of the configurable ones and they can be configured directly on the Session instance.

Signed-off-by: Simone Bordet <[email protected]>
…demand().

* Introduced WebSocket.autoDemand and Session.Listener.AutoDemanding to support automatic demand upon exit of WebSocket handler methods.
* Reworked WebSocketCoreSession implementation to correctly demand after the FrameHandler method invocations, not from the callbacks, and only if autodemanding.
* Fixed implementations of various MessageSinks that were mistakenly demanding, since the demand was already handled by WebSocketCoreSession.

Signed-off-by: Simone Bordet <[email protected]>
@lachlan-roberts lachlan-roberts self-requested a review April 17, 2023 01:52
…ames, that now accept also a Callback.

* Introduced specialized MessageSink implementations to wrap the util.Callback coming from the implementation with an api.Callback.
* Made jetty-websocket-jetty-tests pass.

Signed-off-by: Simone Bordet <[email protected]>
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review (I'm only 56 files in)

sbordet added 3 commits April 18, 2023 00:49
Signed-off-by: Simone Bordet <[email protected]>
…rom jetty-websocket.

Modified ee10 DefaultServlet to avoid NPE during WebSocket tests.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet marked this pull request as ready for review April 21, 2023 15:37
@sbordet
Copy link
Contributor Author

sbordet commented Apr 21, 2023

@gregw @lachlan-roberts @joakime here's my review suggestions:

  • FrameHandler: lost its isAutodemanding() because now itself is responsible to perform demand, so there is no need for others to ask it. If it does not do demand itself, it delegates to MessageSinks.
  • MessageSink and subclasses, that all now take a boolean autoDemand to decide whether to perform demand automatically or not.
  • WebSocketCoreSession: the implementation lost its calls to demand, that now are done by the FrameHandler or MessageSink.
  • core.WebSocketProxy: while being a test class, it shows how things work differently now that the demand is responsibility of the FrameHandler.
  • api.Callback: was previously called WriteCallback but now it is also used for listener methods, so had to drop the Write. It is a bit of the same name and functionality of util.Callback, but the duplication is necessary as we want the WebSocket api to depend only on the JDK classes (for classloading reasons). Open to better names, but WebSocketCallback is a mouthful.
  • api.Session: introduced a demand() method, as well as Listener to replace the 5 listener interfaces of before, removed suspend(). Binary methods in Session.Listener now take an additional Callback parameter.
  • @OnWebSocketConnect: historically this annotation was called in this way (and so the correspondent listener method). However, in jetty-websocket-core the same event is called "open" which I think it's more correct.
  • @OnWebSocketXYZ: should we drop WebSocket and just call them @OnOpen, @OnMessage, etc.? Same for listener methods.
  • @WebSocket now has an autoDemand attribute, correspondent to Session.Listener.AutoDemanding.
  • WebSocketSession now has additional method that were before in Remote which is now removed.

@sbordet sbordet requested review from joakime and gregw April 21, 2023 15:56
sbordet added 4 commits April 24, 2023 17:22
may have not been returned yet when the test was
trying to submit a new message, causing a NPE.

Signed-off-by: Simone Bordet <[email protected]>
* Renamed Session.Listener.onWebSocketConnect() to onWebSocketOpen().
* Updated all references, tests and documentation.

Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
…ding.

* Updated MessageInputStream to fail if an exception is thrown by the application.

Signed-off-by: Simone Bordet <[email protected]>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general lots more javadoc is needed.

I think that FrameHandler.onFrame should return a boolean in the same way that Handler.handle does.

Also would be good to have a bridge to our Content.Source and Content.Sink classes.

Mostly I'm confused as to what autoDemand=false means for MessageSinks - i.e. same question as @joakime .

* {@link CoreSession#demand(long)}.</p>
* <p>Both control and data frames are passed to this method.</p>
* <p>CLOSE frames may be responded from this method, but if
* they are not responded, then the implementation will respond.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the implementation know that the CLOSE will not be responded to?
Should the signature be like our handle method:

    boolean onFrame(Frame frame, Callback callback);

If true is returned then the application will invoke the callback, if false is returned then the application will handle the frame (pong for ping, close for close etc.).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per our call, the close logic is done when the callback is completed, not when the method returns. That is OK.... but needs javadoc.

Comment on lines 34 to 35
* <p>The demand for more frames must be explicitly invoked
* via {@link CoreSession#demand(long)}.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

demand must be invoked by whom? I assume by the MessageSink implementation?

@@ -16,18 +16,17 @@
import java.lang.invoke.MethodHandle;

import org.eclipse.jetty.websocket.core.CoreSession;
import org.eclipse.jetty.websocket.core.Frame;

public class InputStreamMessageSink extends DispatchedMessageSink
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a ContentSourceMessageSink class that supports our Content.Source API.

Copy link
Contributor Author

@sbordet sbordet Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is not possible, as the WebSocket endpoint can only depend on the JDK and the Jetty WebSocket APIs (which in turn only depend on the JDK).

In a webapp I cannot have:

@WebSocket
class MyWSEndpoint {
  @WebSocketMessage
  public void onMessage(Content.Source source) { ... }
}

because Content.Source would need jetty-io in WEB-INF/lib, causing classloader woes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm it is a pity that we can't use our own good IO abstraction. Surely there must be a way? Even if only for embedded usage without a webapp?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MessageSink is part of jetty-websocket-core-common, so it already depends on jetty-io and can see Content.Source.

I understand that it can't be in jetty-api, but then no MessageSink is. I don't see any reason that we can't include a ContentSourceMessageSink

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any reason that we can't include a ContentSourceMessageSink

Because we cannot expose Content.Source to the web application (see above).

All the other MessageSink expose JDK classes such as ByteBuffer, String, InputStream, etc.

We may want to expose java.util.concurrent.Flow.Publisher, but that's different from Content.Source.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MessageSink interface and implementations are in jetty-websocket-core-common, which already depends on jetty-io, so whatever uses message sinks already can see Content.Source.

So embedded usage will be able to use it.

I'm OK with not exposing it in the API... although exposing Flow in the api is a good idea and that can use the ContentSourceMessageSink behind the scenes in the implementation.


public PartialStringMessageSink(CoreSession session, MethodHandle methodHandle)
public PartialStringMessageSink(CoreSession session, MethodHandle methodHandle, boolean autoDemand)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

javadoc on methodHandler and class needed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more needed on autoDemand. See comments elsewhere

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet requested a review from gregw April 26, 2023 15:33
Signed-off-by: Simone Bordet <[email protected]>
Copy link
Contributor

@lachlan-roberts lachlan-roberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for removing the option for the BatchMode in the new Jetty WebSocket API?

*/
String getProtocolVersion();
void close(int statusCode, String reason, Callback callback);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this could use a little more javadoc.

Could be output is still open and we have received a close frame and this decides how to respond. (If no response is sent by the time the callback is succeeded the implementation will attempt to send a close frame for you).

Or if output is closed you may have just received a response close frame.

Or if there was a connection error this will be called with the status 1006 to indicate that no close handshake occurred.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you're suggesting here?
This is the close(int, String, Callback) method, so this method is not called, it's the application that calls it to close.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant to leave this comment on

default void onWebSocketClose(int statusCode, String reason)

accidentally put it in the wrong place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to discuss what to document here, as this method is not clearly defined.

@sbordet sbordet requested a review from lachlan-roberts April 27, 2023 15:04
@sbordet
Copy link
Contributor Author

sbordet commented Apr 28, 2023

What is the reason for removing the option for the BatchMode in the new Jetty WebSocket API?

BatchMode was never used as a tri-state, but only as a boolean, see Jetty 11's JettyWebSocketRemoteEndpoint.isBatch().

In turn that was passed to WebSocket core (which only exposes a boolean API for batch), and eventually arrives to FrameFlusher.
In FrameFlusher, if batch==false, the frame is always written to the network; if batch==true it may or may not be written to the network, depending on internal heuristics.

So for simplicity I removed it, hardcoding the calls to WebSocket core with batch==false; because if an application sets it to true expecting the frame to not be written to the network, it is not what will happen in all cases.
It would also be very difficult to document.

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better... but still needs work. Sorry to be pedantic, but I think this one is important to get very clear.

Comment on lines 31 to 34
* <li>When {@code autoDemand==false}, the {@link MessageSink} manages the
* demand until the conditions to invoke the application function are met;
* when the {@link MessageSink} invokes the application function, then the
* application function is responsible to demand for more WebSocket frames.</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* <li>When {@code autoDemand==false}, the {@link MessageSink} manages the
* demand until the conditions to invoke the application function are met;
* when the {@link MessageSink} invokes the application function, then the
* application function is responsible to demand for more WebSocket frames.</li>
* <li>When {@code autoDemand==false}, the application must initiate demand
* for a message frame and then the {@link MessageSink}, manages the
* demand until the conditions to invoke the application function are met;
* at which point the {@link MessageSink} invokes the application function; and then
* the application is again responsible for initiate any demand for subsequent message frames.</li>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope it's wrong.

What you suggest is completely irrelevant from the point of view of the MessageSink.
Does not matter to document here how the MessageSink is invoked the first time, and less so to document here how "application" code must behave.
For these javadocs needs to be documented how the MessageSink manages the demand after it is invoked the first time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you think the initial demand is irrelevant? To me it is a big difference between the modes: one does the initial demand and the other doesn't.

What is wrong with my version? I think it says much the same as yours (but better) and includes the initial demand.

At the very least, change your version to not use "when" in two different senses. "when the messageSink invokes..." reads like and alternative to "when autodemand==false". Use "then" or some other language that implies a sequence of events.

@@ -18,14 +18,82 @@

import org.eclipse.jetty.websocket.core.CoreSession;

/**
* <p>Partial implementation of {@link MessageSink}.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial suggests partial message

Suggested change
* <p>Partial implementation of {@link MessageSink}.</p>
* <p>Abstract implementation of {@link MessageSink}.</p>

Comment on lines 23 to 29
* <p>The application function passed to the constructor as a {@link MethodHandle}
* either delegates the demand for more WebSocket frames to the {@link MessageSink}
* implementation -- and therefore {@code autoDemand==false}, or explicitly demands
* for more WebSocket frames (via APIs that eventually call
* {@link CoreSession#demand(long)}) -- and therefore {@code autoDemand==true}.</p>
* <p>{@link MessageSink} implementations must handle the demand for WebSocket
* frames in this way:</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not the "application function" that manages demand, but the "application". The "function" might not even be invoked before the initial demand is required to be issued by the application.

Suggested change
* <p>The application function passed to the constructor as a {@link MethodHandle}
* either delegates the demand for more WebSocket frames to the {@link MessageSink}
* implementation -- and therefore {@code autoDemand==false}, or explicitly demands
* for more WebSocket frames (via APIs that eventually call
* {@link CoreSession#demand(long)}) -- and therefore {@code autoDemand==true}.</p>
* <p>{@link MessageSink} implementations must handle the demand for WebSocket
* frames in this way:</p>
* <p>Management of demand for message frames may either be entirely managed
* by the {@link MessageSink} implementation ({@code autoDemand==true}; or
* it may be managed collaboratively between the application and the
* {@link MessageSink} implementation ({@code autoDemand==true}.
* <p>{@link MessageSink} implementations must handle the demand for WebSocket
* frames in this way:</p>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not the "application function" that manages demand, but the "application".

It is that particular application function that manages demand, not some undefined "application".
Also, the demand is not for "message frames" (they don't exist), but for generic WebSocket frames (including CLOSE).

I have reworded into a mix of the two.

sbordet added 2 commits April 29, 2023 17:39
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
* <p>Abstract implementation of {@link MessageSink}.</p>
* <p>Management of demand for WebSocket frames may either be entirely managed
* by the {@link MessageSink} implementation ({@code autoDemand==true}); or
* it may be managed collaboratively between the application function passed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I'm completely stuck on this. I find the "passed application function" entirely confusing. Specifically:

  • how is demand initiated in the first place if that function has not yet been invoked.
  • must demand only be managed by that function? i.e. can demand be called sometime after that function returns?

It makes no sense to me? If the usage of the messageSink is not using autoDemand, then it is likely that demand management is complex and specific to all kind of details within the application, not just the invocation of that one function.

Comment on lines 31 to 34
* <li>When {@code autoDemand==false}, the {@link MessageSink} manages the
* demand until the conditions to invoke the application function are met;
* when the {@link MessageSink} invokes the application function, then the
* application function is responsible to demand for more WebSocket frames.</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you think the initial demand is irrelevant? To me it is a big difference between the modes: one does the initial demand and the other doesn't.

What is wrong with my version? I think it says much the same as yours (but better) and includes the initial demand.

At the very least, change your version to not use "when" in two different senses. "when the messageSink invokes..." reads like and alternative to "when autodemand==false". Use "then" or some other language that implies a sequence of events.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet
Copy link
Contributor Author

sbordet commented Apr 30, 2023

@gregw after a successful WebSocket upgrade, the initial demand is triggered by the "open" event (which means methods annotated with @OnWebSocketOpen, or Session.Listener.onWebSocketOpen()).
At this point, the MessageSink is not in the picture at all: the call to the "open" method does not go through the message sink.

The initial demand may either be automatic or not.
If automatic, the FrameHandler is responsible for it, not the MessageSink.

After the initial demand, the MessageSink will be invoked with data frames (not control frames).
The MessageSink calls the application function depending on the MessageSink implementation logic.

If the WebSocket endpoint is not autodemanding, then the demand is handled by the application function, which either must demand synchronously, or arrange to demand later asynchronously.
Either case, it is managed by the application function, not by MessageSink.

Demand for control frames is managed either by the FrameHandler or by other application functions (e.g. for PING, PONG, CLOSE, etc.).

@sbordet sbordet requested a review from gregw April 30, 2023 15:13
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still like to see a ContentSourceMessageSink that is the implementation behind a Flow.Publisher API. Can you add a TODO for that and we can add in another PR

@sbordet
Copy link
Contributor Author

sbordet commented May 2, 2023

@gregw I opened #9722.

Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@sbordet sbordet merged commit adf5754 into jetty-12.0.x May 2, 2023
@sbordet sbordet deleted the fix/jetty-12-9552-rewrite-websocket-apis branch May 2, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants