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

Fix netty issue #6469

Merged
merged 3 commits into from
Aug 18, 2022
Merged

Fix netty issue #6469

merged 3 commits into from
Aug 18, 2022

Conversation

trask
Copy link
Member

@trask trask commented Aug 13, 2022

App was failing due to:

java.util.NoSuchElementException: io.opentelemetry.javaagent.instrumentation.netty.v4.common.client.NettySslInstrumentationHandler
	at io.netty.channel.DefaultChannelPipeline.getContextOrDie(DefaultChannelPipeline.java:1073)
	at io.netty.channel.DefaultChannelPipeline.addAfter(DefaultChannelPipeline.java:302)
	at io.netty.channel.DefaultChannelPipeline.addAfter(DefaultChannelPipeline.java:290)

because the NettySslInstrumentationHandler was removing itself.

I'm not sure why it was removing itself, but tests still pass without that code, and my test app no longer fails.

@trask trask requested a review from a team as a code owner August 13, 2022 00:10
@mateuszrzeszutek
Copy link
Member

Is there an easy way to reproduce this? I suspect that the real error might be around here: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/NettyChannelPipelineInstrumentation.java#L128
Where we might call addAfter to add a handler after NettySslInstrumentationHandler is no longer there

Removing the NettySslInstrumentationHandler kinda makes sense:

  • the first remove method call is just a sanity check that prevents it from being called if the event class is not on the classpath (should never happen really)
  • the second remove call is there to prevent duplicate ending of the SSL span - this is more or less defensive code, since I'm not sure whether it is even possible to get two of these events. I think we could replace this with setting context = null after ending the span the first time.

Reactor-netty's SslProvider.SslReadHandler does a similar thing, so while deleting the remove() calls might fix the issue, I think we should fix the underlying issue is possible -- it might happen again in some other context.

@trask
Copy link
Member Author

trask commented Aug 17, 2022

@mateuszrzeszutek thx! that makes a lot of sense, I pushed a new fix

@trask trask added this to the v1.17.0 milestone Aug 18, 2022
@@ -20,6 +22,9 @@ public final class NettySslInstrumentationHandler extends ChannelDuplexHandler {
private static final Class<?> SSL_HANDSHAKE_COMPLETION_EVENT;
private static final MethodHandle GET_CAUSE;

private static final VirtualField<ChannelHandler, ChannelHandler> instrumentationHandlerField =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps here should be a comment that the same virtual field is used elsewhere. Currently it is not obvious what it is for because in this file this virtual field is only set to null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants