Use one websocket per browser client #2606
Conversation
niloc132
force-pushed
the
2510-multiplexed-websocket
branch
from
6d4456e to
7dc9045
Compare
|
Note that this is dependent on improbable-eng/grpc-web#1112 (and a subsequent release) to correctly allow clients of a netty server to use this client. Alternatively, we could build our own jetty grpc proxy, or build the JS output differently for netty than jetty so that it defaults to non-multiplexed, or drop support for netty as a server entirely. |
niloc132
force-pushed
the
2510-multiplexed-websocket
branch
2 times, most recently
from
808fd51 to
1cc40d8
Compare
niloc132
force-pushed
the
2510-multiplexed-websocket
branch
from
ba25c9b to
a6e850b
Compare
niloc132
changed the title
| Map<String, Supplier<Endpoint>> endpoints = Map.of( | ||
| "grpc-websockets", () -> filter.create(WebSocketServerStream::new), | ||
| "grpc-websockets-multiplex", () -> filter.create(MultiplexedWebSocketServerStream::new)); | ||
| container.addEndpoint(ServerEndpointConfig.Builder.create(GrpcWebsocket.class, "/{service}/{method}") | ||
| .configurator(new ServerEndpointConfig.Configurator() { | ||
| @Override | ||
| public <T> T getEndpointInstance(Class<T> endpointClass) throws InstantiationException { | ||
| // noinspection unchecked | ||
| return (T) new GrpcWebsocket(endpoints); | ||
| } | ||
| }) | ||
| .subprotocols(Arrays.asList("grpc-websockets", "grpc-websockets-multiplex")) | ||
| .build() |
There was a problem hiding this comment.
Should we move the string constants into GrpcWebsocket, and simplify the constructor to GrpcWebsocket(Supplier<Endpoint> vanilla, Supplier<Endpoint> multiplexed)? A bit more explicit interface between parts IMO.
| if (frame != null) { | ||
| int numBytes = frame.readableBytes(); | ||
| if (numBytes > 0) { | ||
| onSendingBytes(numBytes); |
There was a problem hiding this comment.
should this be numBytes + 4?
There was a problem hiding this comment.
No, this is for grpc internally to decide if it should send more. We don't tell it about http2 framing bytes, so we likewise shouldnt tell it about ws framing.
| payload.flip(); | ||
|
|
||
| websocketSession.getBasicRemote().sendBinary(payload); | ||
| transportState.runOnTransportThread(() -> transportState.onSentBytes(numBytes)); |
There was a problem hiding this comment.
Should this be numBytes + 4?
There was a problem hiding this comment.
As above, I don't think that would work.
...cket-jakarta/src/main/java/io/grpc/servlet/web/websocket/MultiplexedWebsocketStreamImpl.java
Outdated
Show resolved
Hide resolved
...cket-jakarta/src/main/java/io/grpc/servlet/web/websocket/MultiplexedWebsocketStreamImpl.java
Show resolved
Hide resolved
...et-jakarta/src/main/java/io/grpc/servlet/web/websocket/MultiplexedWebSocketServerStream.java
Outdated
Show resolved
Hide resolved
...et-jakarta/src/main/java/io/grpc/servlet/web/websocket/MultiplexedWebSocketServerStream.java
Outdated
Show resolved
Hide resolved
Creates a new websocket protocol to let the websocket behave only as a transport on which multiple streams, across services, can be conveyed. The websocket will be opened to the path matching the first stream, but after the websocket has started, other streams can be sent on it, using custom metadata/header to give the path to the desired rpc.
By reusing existing paths, the client can signal that it supports either old or new websockets, so it in theory can connect to the old grpc-websockets implementation. Unfortunately, the reference implementation can't handle alternative requested subprotocols, and will just fail the entire request, so this patch breaks our compatibility with that proxy, and so with our netty implementation.
On the other hand, the server will maintain compatibility for both protocols for now, enabling stale js api clients to still be able to connect. We may also find that there are issues with longlived sockets, and might end up wanting to ask the client in some circumstances to use the old implementation.
Fixes #2510