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

ThreadContextElements are not nested properly #2195

Closed
akalini opened this issue Aug 12, 2020 · 1 comment
Closed

ThreadContextElements are not nested properly #2195

akalini opened this issue Aug 12, 2020 · 1 comment
Assignees

Comments

@akalini
Copy link

akalini commented Aug 12, 2020

Hello!

For some of the implementations of the ThreadContextElements the order of invocation is important. Here is one example that we are struggling with and don't know how to solve cleanly:

  • Kotlin gRPC library has GrpcContextElement that propagates gRPC context.
  • The element above attaches and detaches gRPC context
  • We have additional gRPC context elements that we want to propagate. This is to support opencensus trace contexts which uses gRPC context for the implementation.
  • Kotlin coroutines library invokes restoreThreadContext in exactly the same order as updateThreadContext.
  • But the order in which detach calls happen need to be the opposite of attach calls (required by gRPC).

Is there a reason why the order for restoreThreadContext is not the opposite of updateThreadContext?

@qwwdfsad
Copy link
Member

qwwdfsad commented Aug 13, 2020

Hi,

Is there a reason why the order for restoreThreadContext is not the opposite of updateThreadContext?

This behaviour is implementation-driven, at that point it was the most efficient way to implement it in the most performant way. Both updateThreadContext and restoreThreadContext are performance-sensitive because they are invoked on each resume and suspend. Your concern wasn't really considered back then.

Thanks for the report, this is definitely an issue worth re-considering for 1.4

@qwwdfsad qwwdfsad self-assigned this Aug 13, 2020
qwwdfsad added a commit that referenced this issue Feb 2, 2021
    * Restore the context in the reverse order of update, so they are properly nested into each other
    * Also, do a minor cleanup

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

No branches or pull requests

2 participants