Ensure ChannelHandlerContext.isRemoved is called only when in event loop

[violetagg]

Related to #2981

[pderop]

How do you feel about reducing the code duplications between the http2FrameCodecCtx()/http2MultiplexHandlerCtx()/h2cUpgradeHandlerCtx() method:
instead of the current:

		@Nullable
		ChannelHandlerContext http2FrameCodecCtx() {
			ChannelHandlerContext ctx = http2FrameCodecCtx;
			// ChannelHandlerContext.isRemoved is only meant to be called from within the EventLoop
			if (ctx != null && connection.channel().eventLoop().inEventLoop() && !ctx.isRemoved()) {
				return ctx;
			}
			ctx = connection.channel().pipeline().context(Http2FrameCodec.class);
			http2FrameCodecCtx = ctx;
			return ctx;
		}

		@Nullable
		ChannelHandlerContext http2MultiplexHandlerCtx() {
			ChannelHandlerContext ctx = http2MultiplexHandlerCtx;
			// ChannelHandlerContext.isRemoved is only meant to be called from within the EventLoop
			if (ctx != null && connection.channel().eventLoop().inEventLoop() && !ctx.isRemoved()) {
				return ctx;
			}
			ctx = connection.channel().pipeline().context(Http2MultiplexHandler.class);
			http2MultiplexHandlerCtx = ctx;
			return ctx;
		}

		@Nullable
		ChannelHandlerContext h2cUpgradeHandlerCtx() {
			ChannelHandlerContext ctx = h2cUpgradeHandlerCtx;
			// ChannelHandlerContext.isRemoved is only meant to be called from within the EventLoop
			if (ctx != null && connection.channel().eventLoop().inEventLoop() && !ctx.isRemoved()) {
				return ctx;
			}
			ctx = connection.channel().pipeline().context(NettyPipeline.H2CUpgradeHandler);
			h2cUpgradeHandlerCtx = ctx;
			return ctx;
		}

then do a variant like this (I have verified by doing a benchmark and the performance is similar):

		@Nullable
		ChannelHandlerContext http2FrameCodecCtx() {
			return getCachedHandlerContext(http2FrameCodecCtx, () ->
					http2FrameCodecCtx = connection.channel().pipeline().context(Http2FrameCodec.class));
		}

		@Nullable
		ChannelHandlerContext http2MultiplexHandlerCtx() {
			return getCachedHandlerContext(http2MultiplexHandlerCtx, () ->
					http2MultiplexHandlerCtx = connection.channel().pipeline().context(Http2MultiplexHandler.class));
		}

		@Nullable
		ChannelHandlerContext h2cUpgradeHandlerCtx() {
			return getCachedHandlerContext(h2cUpgradeHandlerCtx, () ->
					h2cUpgradeHandlerCtx = connection.channel().pipeline().context(NettyPipeline.H2CUpgradeHandler));
		}

		@Nullable
		ChannelHandlerContext getCachedHandlerContext(ChannelHandlerContext ctx, Supplier<ChannelHandlerContext> getCtx) {
			// ChannelHandlerContext.isRemoved is only meant to be called from within the EventLoop
			if (ctx != null && connection.channel().eventLoop().inEventLoop() && !ctx.isRemoved()) {
				return ctx;
			}
			return getCtx.get();
		}

or the above could be considered in another enhancement PR ?
in any case, +1 for the patch.

[violetagg]

I was thinking about this, but isn't this generating too many unnecessary objects in a hot path?

[pderop]

I just verified with a comparison benchmark, it's really similar, it even goes a bit faster, and lambda/invokedynamic do not allocate objects if I'm correct.
well, we can consider this in another PR on any doubts.

[violetagg]

Interesting ... I might be missing something but you are using the fields of the enclosing class, aren't you?

invocation 1

invocation 2

[pderop]

Yes. to be clear, here is a similar thing, but with lambdas enclosed in braces:

		@Nullable
		ChannelHandlerContext http2FrameCodecCtx() {
			return getCachedHandlerContext(http2FrameCodecCtx, () -> {
				return http2FrameCodecCtx = connection.channel().pipeline().context(Http2FrameCodec.class);
			});
		}

		@Nullable
		ChannelHandlerContext http2MultiplexHandlerCtx() {
			return getCachedHandlerContext(http2MultiplexHandlerCtx, () -> {
				return http2MultiplexHandlerCtx = connection.channel().pipeline().context(Http2MultiplexHandler.class);
			});
		}

		@Nullable
		ChannelHandlerContext h2cUpgradeHandlerCtx() {
			return getCachedHandlerContext(h2cUpgradeHandlerCtx, () -> {
					return h2cUpgradeHandlerCtx = connection.channel().pipeline().context(NettyPipeline.H2CUpgradeHandler);
			});
		}

		@Nullable
		ChannelHandlerContext getCachedHandlerContext(ChannelHandlerContext ctx, Supplier<ChannelHandlerContext> getCtx) {
			// ChannelHandlerContext.isRemoved is only meant to be called from within the EventLoop
			if (ctx != null && connection.channel().eventLoop().inEventLoop() && !ctx.isRemoved()) {
				return ctx;
			}
			return getCtx.get();
		}

is there any concerns about the above or about the initial proposal ?
The intention was to avoid the code duplication, but maybe I unintentionally planted a coding potato 😂 ?
if this not clear, you can forget about it, and I can give another try in a future PR.

[pderop]

mmm, my bad, this morning, I was reviewing the previous comments, and the lambdas are, as you mentioned, accessing instance fields of the surrounding class, and if correct, this requires capturing the value of the this reference.
and we can see allocations are taking place from your screenshots.

Please forget about the proposed enhancement! 😊

[violetagg]

@pderop Thanks for the review!