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 RequestContext propagation and callback invocation #220

Merged
merged 1 commit into from
Aug 9, 2016

Conversation

trustin
Copy link
Member

@trustin trustin commented Aug 9, 2016

Motivation:

We currently use RequestContext.push(..) to propagate a RequestContext
to a non-I/O thread or to replace the current context with another.

When propagating a RequestContext a non-I/O thread, the onEnter and
onExit callbacks must be invoked, while they should not be invoked when
replacing a context.

RequestContext.push(...) currently never invokes the callbacks, which
can lead to inconsistent state transition between threads.

Modifications:

  • Modify RequestContext.push() so that the caller can choose to call the
    callbacks, where the default behavior is to call the callback
    • Add RequestContext.invokeOnEnter/ExitCallbacks()
  • Reorganize the type hierarchy of RequestContext and its subtypes
    • Split AbstractRequestContext into:
      • AbstractRequestContext
      • NonWrappingRequestContext extends AbstractRequestContext
      • Default*RequestContext extend NonWrappingRequestContext
    • Add RequestContextWrapper that extend AbstractRequestContext
      • Rewrite ServiceRequestContextWrapper using RequestContextWrapper
      • Add ClientRequestContextWrapper
  • Fix a bug where THttpService does not push its context when calling
    its delegate such as ThriftCallService
  • Fix a bug where ThriftCallService pushes its context unnecessarily
  • Fix a bug where AbstractCompositeService does not push its context
    wrapper
  • Add HttpTracingIntegrationTest which ensures that Zipkin/Brave's
    thread-local variables are transferred correctly.
  • Miscellaneous
    • Unignore RequestContextTest.contextAwareEventExecutor()

Result:

  • RequestContext propagation works as expected
  • Call tracing of nested calls works as expected

@trustin trustin added this to the 0.21.3.Final milestone Aug 9, 2016
@trustin trustin added the defect label Aug 9, 2016
@trustin trustin force-pushed the fix_context_propagation branch from 2bde003 to b07d655 Compare August 9, 2016 06:29
@trustin
Copy link
Member Author

trustin commented Aug 9, 2016

/cc @anuraaga @synk

@anuraaga
Copy link
Collaborator

anuraaga commented Aug 9, 2016

LGTM

@codecov-io
Copy link

codecov-io commented Aug 9, 2016

Current coverage is 64.11% (diff: 60.37%)

Merging #220 into master will increase coverage by 0.58%

@@             master       #220   diff @@
==========================================
  Files           274        277     +3   
  Lines         10523      10574    +51   
  Methods           0          0          
  Messages          0          0          
  Branches       1520       1525     +5   
==========================================
+ Hits           6686       6780    +94   
+ Misses         3099       3059    -40   
+ Partials        738        735     -3   

Powered by Codecov. Last update 21688e0...c9b17f3

@kojilin
Copy link
Contributor

kojilin commented Aug 9, 2016

LGTM~

Motivation:

We currently use RequestContext.push(..) to propagate a RequestContext
to a non-I/O thread or to replace the current context with another.

When propagating a RequestContext a non-I/O thread, the onEnter and
onExit callbacks must be invoked, while they should not be invoked when
replacing a context.

RequestContext.push(...) currently never invokes the callbacks, which
can lead to inconsistent state transition between threads.

Modifications:

- Modify RequestContext.push() so that the caller can choose to call the
  callbacks, where the default behavior is to call the callback
  - Add RequestContext.invokeOnEnter/ExitCallbacks()
- Reorganize the type hierarchy of RequestContext and its subtypes
  - Split AbstractRequestContext into:
    - AbstractRequestContext
    - NonWrappingRequestContext extends AbstractRequestContext
    - Default*RequestContext extend NonWrappingRequestContext
  - Add RequestContextWrapper that extend AbstractRequestContext
    - Rewrite ServiceRequestContextWrapper using RequestContextWrapper
    - Add ClientRequestContextWrapper
- Fix a bug where THttpService does not push its context when calling
  its delegate such as ThriftCallService
- Fix a bug where ThriftCallService pushes its context unnecessarily
- Fix a bug where AbstractCompositeService does not push its context
  wrapper
- Add HttpTracingIntegrationTest which ensures that Zipkin/Brave's
  thread-local variables are transferred correctly.
- Miscellaneous
  - Unignore RequestContextTest.contextAwareEventExecutor()

Result:

- RequestContext propagation works as expected
- Call tracing of nested calls works as expected
@trustin trustin force-pushed the fix_context_propagation branch from b07d655 to c9b17f3 Compare August 9, 2016 06:53
@synk
Copy link
Contributor

synk commented Aug 9, 2016

I checked the tests, and LGTM

@trustin
Copy link
Member Author

trustin commented Aug 9, 2016

Thank's for reviewing @anuraaga @kojilin and @synk

@trustin trustin merged commit eb3f036 into line:master Aug 9, 2016
@trustin trustin deleted the fix_context_propagation branch August 9, 2016 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants