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

ApplicationProtocolNegotiationHandler loses its place in the context too soon #9131

Closed
RoganDawes opened this issue May 7, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@RoganDawes
Copy link
Contributor

commented May 7, 2019

Expected behavior

	@Override
	protected void configurePipeline(ChannelHandlerContext ctx, String protocol)
			throws Exception {
		ctx.pipeline().replace(this, null, initializer);
		// or
		ctx.pipeline().addAfter(this, null, initializer);
		// or
		ctx.pipeline().addBefore(this, null, initializer);
	}

should work, similarly to how a ChannelInitializer works.

Actual behavior

An exception is thrown on all "relative" pipeline operations, because the ApplicationProtocolNegotiationHandler instance has already been removed from the pipeline BEFORE configurePipeline() is called:

    @Override
    public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception {
        if (evt instanceof SslHandshakeCompletionEvent) {
            ctx.pipeline().remove(this); // <-- Removed before calling configurePipeline :-(

            SslHandshakeCompletionEvent handshakeEvent = (SslHandshakeCompletionEvent) evt;
            if (handshakeEvent.isSuccess()) {
                SslHandler sslHandler = ctx.pipeline().get(SslHandler.class);
                if (sslHandler == null) {
                    throw new IllegalStateException("cannot find a SslHandler in the pipeline (requir
ed for " +
                                                    "application-level protocol negotiation)");
                }
                String protocol = sslHandler.applicationProtocol();
                configurePipeline(ctx, protocol != null? protocol : fallbackProtocol);
            } else {
                handshakeFailure(ctx, handshakeEvent.cause());
            }
        }

        ctx.fireUserEventTriggered(evt);
    }

Compare that to Channelinitializer:

    @SuppressWarnings("unchecked")
    private boolean initChannel(ChannelHandlerContext ctx) throws Exception {
        if (initMap.add(ctx)) { // Guard against re-entrance.
            try {
                initChannel((C) ctx.channel());
            } catch (Throwable cause) {
                // Explicitly call exceptionCaught(...) as we removed the handler before calling init
Channel(...).
                // We do so to prevent multiple calls to initChannel(...).
                exceptionCaught(ctx, cause);
            } finally {
                ChannelPipeline pipeline = ctx.pipeline();
                if (pipeline.context(this) != null) {
                    pipeline.remove(this);
                }
            }
            return true;
        }
        return false;
    }

Note that the comment regarding the handler already having been removed in Channelinitializer is incorrect. See the following excerpt from current code:

    /**
     * {@inheritDoc} If override this method ensure you call super!
     */
    @Override
    public void handlerAdded(ChannelHandlerContext ctx) throws Exception {
        if (ctx.channel().isRegistered()) {
            // This should always be true with our current DefaultChannelPipeline implementation.
            // The good thing about calling initChannel(...) in handlerAdded(...) is that there will be no ordering
            // surprises if a ChannelInitializer will add another ChannelInitializer. This is as all handlers
            // will be added in the expected order.
            if (initChannel(ctx)) {

                // We are done with init the Channel, removing the initializer now.
                removeState(ctx);
            }
        }
    }

Steps to reproduce

Use ctx.replace(...) in the configurePipeline() method.

Minimal yet complete reproducer code (or URL to code)

Not available.

Netty version

netty-4.1.36.Final

JVM version (e.g. java -version)

java version "11.0.1" 2018-10-16 LTS
Java(TM) SE Runtime Environment 18.9 (build 11.0.1+13-LTS)
Java HotSpot(TM) 64-Bit Server VM 18.9 (build 11.0.1+13-LTS, mixed mode)

OS version (e.g. uname -a)

Linux nemesis 4.15.0-48-generic #51-Ubuntu SMP Wed Apr 3 08:28:49 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

@RoganDawes

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Since ApplicationProtocolNegotiationHandler is just conceptually a special case of a Channelinitializer, similar logic should be used when invoking configurePipeline(), and patterns possible in ChannelInitializer should also be possible in ApplicationProtocolNegotiationHandler. The only real difference is that the invocation is not triggered by the handler being added, but rather some event a bit further down the line.

@normanmaurer

This comment has been minimized.

Copy link
Member

commented May 7, 2019

@RoganDawes sounds like a bug... are you interested in providing a PR ?

@RoganDawes

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@normanmaurer normanmaurer self-assigned this May 10, 2019

@normanmaurer normanmaurer modified the milestones: def, 4.1.37.Final May 10, 2019

@normanmaurer normanmaurer added the defect label May 10, 2019

normanmaurer added a commit that referenced this issue May 13, 2019

Remove the Handler only after it has initialized the channel (#9132)
Motivation:

Previously, any 'relative' pipeline operations, such as
ctx.pipeline().replace(), .addBefore(), addAfter(), etc
would fail as the handler was not present in the pipeline.

Modification:

Used the pattern from ChannelInitializer when invoking configurePipeline().

Result:

Fixes #9131

normanmaurer added a commit that referenced this issue May 13, 2019

Remove the Handler only after it has initialized the channel (#9132)
Motivation:

Previously, any 'relative' pipeline operations, such as
ctx.pipeline().replace(), .addBefore(), addAfter(), etc
would fail as the handler was not present in the pipeline.

Modification:

Used the pattern from ChannelInitializer when invoking configurePipeline().

Result:

Fixes #9131
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.