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

Decouple WebSocket server extension handshaker from read I/O #8973

Closed
wants to merge 1 commit into from

Conversation

@kachayev
Copy link
Contributor

kachayev commented Mar 23, 2019

Motivation:

WebSocketServerExtensionHandler turns extensions on by examining the channelRead event for a HttpRequest message. It means that the instance of a handler should be present in the pipeline before the request comes in.

But let's assume I have a server that covers a lot of different requests, e.g. I have a routing mechanism that decides what should be invoked based on URI provided. If I want to perform WebSocket upgrade only at "/websocket/*", I don't what to pay the price of the handler sitting in the pipeline all the time. It's better to include it only when I know exactly this should be a WebSocket connection. In such a case I've already read and decoded the request from the channel and I need a way to provide this request to the handler not through firing I/O events.

Modifications:

Added a new contructor with additional HTTP request argument to WebSocketServerExtensionHandler and WebSocketServerCompressionHandler.

Ideally, the implementation should also prevent reading 2 pipelined requests consequently. But it was not covered explicitly before, so I didn't change the details (this should not be a problem as the connection would be closed anyway because of a decoder exception).

Result:

More flexible WebSocketServerExtensionHandler API.

Motivation:

`WebSocketServerExtensionHandler` which turns extensions on by examining
the `channelRead` event for a `HttpRequest` message. It means that the
instance of a handler should be present in the pipeline **before** the request
comes in. But let's assume I have a server that covers a lot of different
requests, e.g. I have a routing mechanism that decides what should be invoked
based on URI provided. If I want to perform WebSocket upgrade only at
"/websocket/*", I don't what to pay the price of the handler sitting in the
pipeline all the time. It's better for me to include only when I know exactly
that this should be a WebSocket connection. In such a case I've already
read and decoded the request from the channel and I need a way to provide
this request to the handler not through firing I/O events.

Modifications:

Added a new contructor with additional HTTP request argument to
`WebSocketServerExtensionHandler` and `WebSocketServerCompressionHandler`.

Ideally, the implementation should also prevent reading 2 pipelined requests
consequently. But it was not covered explecitly before, so I didn't change the
details (this should not be a problem as the connection would be closed anyways
because of a decoder exception).

Result:

More flexible `WebSocketServerExtensionHandler` API.
@netty-bot

This comment has been minimized.

Copy link

netty-bot commented Mar 23, 2019

Can one of the admins verify this patch?

@amizurov

This comment has been minimized.

Copy link
Contributor

amizurov commented Mar 24, 2019

@kachayev I think that these thoughts: It means that the instance of a handler should be present in the pipeline before the request comes in. is not correct. You can add WebSocketServerProtocolHandler in runtime in your path router or something like this. We have both protocols (http, ws) in our production system working simultaneously and no problem with it.

The real problem on client side - WebSocketClientProtocolHandshakeHandler that initiate handshake request only when triggering channelActive(). It's mean if you added WebSocketClientProtocolHandler in runtime, then nothing will happen and you need call fireChannelActive() for starting upgrade.

@kachayev

This comment has been minimized.

Copy link
Contributor Author

kachayev commented Mar 24, 2019

@amizurov You can, but you need to fireChannelRead from the correct position hoping it won't make any harm further in the pipeline that already consists of the handlers tailored to WebSocket communication.

@amizurov

This comment has been minimized.

Copy link
Contributor

amizurov commented Mar 24, 2019

Yes, but fireChannelRead is simple continuation of pipeline execution. For example:

public class WebSocketInitializerHandler extends SimpleChannelInboundHandler<FullHttpRequest> {

    private final String wsPath;

    public WebSocketInitializerHandler(String wsPath) {
        super(false);
        this.wsPath = checkNotNull(wsPath, "wsPath");
    }

    @Override
    protected void channelRead0(ChannelHandlerContext ctx, FullHttpRequest request) throws Exception {
        if (HttpMethod.GET.equals(request.method()) && request.uri().equals(wsPath)) {
            ctx.pipeline()
               .addAfter(ctx.name(), "wsCompression", new WebSocketServerCompressionHandler())
               .addAfter("wsCompression", "wsHandler", new WebSocketServerProtocolHandler(wsPath, null, true));
        }

        ctx.fireChannelRead(request);
    }

}
@kachayev

This comment has been minimized.

Copy link
Contributor Author

kachayev commented Mar 24, 2019

@amizurov Yeah, sure. But that's specifically what I'm trying to avoid. I don't use WebSocketServerProtocolHandler in order to manage handshake "manually" (because of a few sharp corners). To avoid firing request into the pipeline that mostly doesn't care about it I need to do something like rebuild pipeline -> fire read -> rebuild once again. Which seems to be unnecessary fragile. Another solution to the problem would be to extend handshaker to cover extensions handshake when necessary. The good reason for doing this is that you create handshaker passing the instance of request object to the factory explicitly not through the pipeline. The problem tho' is that the current implementation of extension handshaker is coupled with pipeline processing.

public WebSocketServerExtensionHandler(HttpRequest request,
WebSocketServerExtensionHandshaker... extensionHandshakers) {
this(extensionHandshakers);
processHttpRequest(request);

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Mar 28, 2019

Member

I didn't look at the whole PR yet (sorry super busy) but I really dont like to call a private method from the constructor. This looks more like a code smell than a code thing to me.

This comment has been minimized.

Copy link
@kachayev

kachayev Mar 29, 2019

Author Contributor

Agree. The only way to work this around I can think of is to have a static factory method, the same way it works for creating a new handshaker providing the instance of a request (the problem remains tho', just being re-shaped a bit). Maybe you can suggest another approach here from a coding perspective?

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Apr 8, 2019

@kachayev as stated before I am not a fan of this change... Why you not just use a EmbeddedChannel for this if you are concerned about the overhead of having the handler in the pipeline all the time ?

@kachayev

This comment has been minimized.

Copy link
Contributor Author

kachayev commented Apr 9, 2019

@normanmaurer Yeah, absolutely. There are some ways to work this around. I was mostly concerned with different API approaches: websocket handshaker is done by a factory that takes a request object, but websocket extension handshaker at the same time should be a part of the pipeline and it should catch exactly the same instance of a request through the pipeline read/write.

@kachayev kachayev closed this Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.