-
Notifications
You must be signed in to change notification settings - Fork 860
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 context propagation in Executor#execute() for non-capturing lambdas #9179
Fix context propagation in Executor#execute() for non-capturing lambdas #9179
Conversation
I think there was something that failed with the decorator approach. Some |
...src/main/java/io/opentelemetry/javaagent/bootstrap/executors/ContextPropagatingRunnable.java
Show resolved
Hide resolved
…etry/javaagent/bootstrap/executors/ContextPropagatingRunnable.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
// We wrap only lambdas' anonymous classes and if given object has not already been wrapped. | ||
// Anonymous classes have '/' in class name which is not allowed in 'normal' classes. | ||
// note: it is always safe to decorate lambdas since downstream code cannot be expecting a specific runnable implementation anyways | ||
return task.getClass().getName().contains("/") && !(task instanceof ContextPropagatingRunnable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can handle the "lambda" situation, but how about the singleton task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR does not fix that. We're planning to redesign the executors
instrumentation and fix that issue across the board; but that might take a while and will not be included in the next release (which is in ~ 5 days).
…as (open-telemetry#9179) Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Fixes #9175
This is an interesting issue that only occurs when using a non-capturing lambda (or any other singleton/cached
Runnable
instance), and only whenExecutor#execute()
is used (since all otherExecutorService
methods typically wrap the passed task in a newFutureTask
and delegate toexecute()
anyway).Because a singleton
Runnable
instance was passed, the whole machinery inExecutorAdviceHelper
andTaskAdviceHelper
that handles a stateful virtual field completely broke when trying to correctly update thePropagatedContext
, and sometimes overwrote the propagated context with anull
.While this is a surgical fix that handles precisely the issue posted by the user, I believe we should rethink our executors instrumentation as a whole: I think instead of the complex and buggy state management we have right now we should switch to decoration whenever it's possible. While this will add a little bit of extra memory overhead, I think it's much safer that way.