Make Http2MultiplexHandler construction require explicit distinction between server and client

[yawkat]

Motivation:

Because Http2MultiplexHandler attempts to infer isServer from the channel type (ServerChannel), it can't be used in an EmbeddedChannel for HTTP server testing.

Modification:

Deprecate the old Http2MultiplexHandler constructors, and replace them with forClient and forServer factory methods, similar to Http2FrameCodecBuilder.

Result:

• Http2MultiplexHandler can be used in an EmbeddedChannel
• construction is a bit more clear, as you can't pass an upgradeHandler to a server multiplexer anymore, where it would never be used anyway

[normanmaurer]

only one nit.. Looks great otherwise

[mostroverkhov]

is deprecation actually necessary? This change helps for small edge case (testing, with EmbeddedChannel only - but many use real channels for testing), otherwise current API is sufficient. It would force netty users onto new API for no practical gain.

[yawkat]

it's not like they have to be removed in 4.x, but it seems weird to have both the factory and the constructor.

[mostroverkhov]

is deprecation actually necessary? This change helps for small edge case (testing, with EmbeddedChannel only - but many use real channels for testing), otherwise current API is sufficient. It would force netty users onto new API for no practical gain.

[mostroverkhov]

@yawkat also wondering if public class EmbeddedServerChannel extends EmbeddedChannel implements ServerChannel would work for tests, instead of updating http2 handlers API for testing purposes.

[yawkat]

Yes it's a possible workaround, but it seems like a worse solution.

[mostroverkhov]

It addresses the root cause - aims to solve testing problem by extending test code. And is alternative to patching 1 production codec with new API to sidestep the fact that test EmbeddedChannel (this is connection mock) does not have parent channel (on server side, this is connection factory).

So for "clean" solution you should instead have

static class EmbeddedServerChannel extends EmbeddedChannel implements ServerChannel {
 }

and add ctor to EmbeddedChannel that accepts parentChannel

public EmbeddedChannel(Channel parent) {
       this(parent, EmbeddedChannelId.INSTANCE, true, false, EMPTY_HANDLERS);
}

I am pretty sure your test then passes with

EmbeddedChannel embeddedChannel = new EmbeddedChannel(new EmbeddedServerChannel());

If you want to fix the problem today w/o patching netty at all, then

static class EmbeddedServerChannel extends EmbeddedChannel implements ServerChannel {

        @Override
        public Channel parent() {
            return this;
        }
    }

Hope this explanation is helpful.

[normanmaurer]

It addresses the root cause - aims to solve testing problem by extending test code. And is alternative to patching 1 production codec with new API to sidestep the fact that test EmbeddedChannel (this is connection mock) does not have parent channel (on server side, this is connection factory).

So for "clean" solution you should instead have

static class EmbeddedServerChannel extends EmbeddedChannel implements ServerChannel {
 }

and add ctor to EmbeddedChannel that accepts parentChannel

public EmbeddedChannel(Channel parent) {
       this(parent, EmbeddedChannelId.INSTANCE, true, false, EMPTY_HANDLERS);
}

I am pretty sure your test then passes with

EmbeddedChannel embeddedChannel = new EmbeddedChannel(new EmbeddedServerChannel());

If you want to fix the problem today w/o patching netty at all, then

static class EmbeddedServerChannel extends EmbeddedChannel implements ServerChannel {

        @Override
        public Channel parent() {
            return this;
        }
    }

Hope this explanation is helpful.

Actually there is already a constructor that takes a parent. So with this in mind we should just close this. Thanks @mostroverkhov for stepping in

[yawkat]

Note that EmbeddedChannel is not used only for testing. And there are other byte-carrying channels too (including the ones introduced by this multiplex handler!), where adding the marker interface is not feasible. By the same reasoning, we could change Http2FrameCodecBuilder to deduce the server-ness from the context, instead of the forClient/forServer approach. Why use deduction in one case but not in others?

I also don't see the downside to this change. It does not remove any methods. And the class is even @UnstableApi.

[normanmaurer]

@yawkat while I agree that the change is not "bad" I also think that there is no really "need / benefit" for the change.

[normanmaurer]

/cc @chrisvest @trustin thoughts ?

[yawkat]

Fair enough. We can always reconsider if someone else hits this in a situation where a workaround is harder.

[chrisvest]

There are not many cases where a server-or-not check comes up. Making EmbeddedChannel behave more similar to other channels (by adding a server-sibling) is the better option, in my opinion. For Netty 5 we can add an isServer method to Channel so we no longer need to infer this information, but can just ask (and override if need be).

[yawkat]

I think there are a few problems with getting the "serverness" from the channel in general (though not all apply here). - it doesn't make sense for all channels, eg it doesn't for udp (or, well, embedded channels) - for channels where it makes some sense, it may still be useful to customize on a per-handler basis. Eg why shouldn't a user be able to run a http2 connection in reverse, with the tcp server acting as the http2 client? Maybe not terribly useful, but it's still an arbitrary restriction. Right now, basically the only constraint a handler imposes on the user in terms of where the handler can be used is the "direction" of the handler (is it outbound or inbound). Relying on the channel for inferring "serverness" would add another constraint. - it's less convenient in the handler implementation to have to infer from the channel, because you have to wait for handlerAdded (also limits sharability). The http2 frame codec is another example for this, since it already has the forClient and forServer versions. Adjusting that to infer from the channel would be harder to implement. - explicit factories that differ for client and server allow for different parameter sets, which makes the api cleaner. This multiplex handler is a good example of this: in its current version, even the server variant accepts an upgradeHandler, even though it is only used by the client variant. This is not ideal api design. These are some reason why I don't like this channel inference approach (though it's not a big issue in a limited case like this pr), and why I don't think "formalizing" it through an explicit Channel.isServer (with the implication that more handlers will do inference in the future) is a good idea.

…

On Tue, Jul 5, 2022, 22:39 Chris Vest ***@***.***> wrote: There are not many cases where a server-or-not check comes up. Making EmbeddedChannel behave more similar to other channels (by adding a server-sibling) is the better option, in my opinion. For Netty 5 we can add an isServer method to Channel so we no longer need to infer this information, but can just ask (and override if need be). — Reply to this email directly, view it on GitHub <#12546 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASIZII24EMBAUBYJGRQKLLVSSMONANCNFSM52MFXPDQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[yawkat]

@chrisvest @normanmaurer I've found another case where this check fails. When you get a SocketChannel from inetd or systemd with socket activation (either through System.inheritedChannel(), or through new EpollSocketChannel(fd)), then the channel won't have a ServerChannel parent even though it is a server-side channel. For nio this can probably be fixed by using new NioSocketChannel(new FakeServerChannel(), (SocketChannel) System.inheritedChannel()) where FakeServerChannel is an EmbeddedChannel that implements ServerChannel as described above. However for epoll no such constructor exists, and it is again an ugly solution anyway.