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

Propagate RequestAttributes across Hystrix Thread boundary #1379

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@taxone

taxone commented Oct 5, 2016

This code fixes this open issue.
The problem with OAuth2 is that Hystrix threads can't access the request/session scoped beans stored by Spring in the main thread, so it's necessary to copy the attributes managed by the Spring RequestContextHolder from the main thread to the threads managed by Hystrix thread pool.

Claudio Tasso
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 5, 2016

Current coverage is 73.95% (diff: 100%)

Merging #1379 into master will decrease coverage by 0.70%

@@             master      #1379   diff @@
==========================================
  Files           187        185     -2   
  Lines          5751       5683    -68   
  Methods           0          0          
  Messages          0          0          
  Branches        864        862     -2   
==========================================
- Hits           4294       4203    -91   
- Misses         1149       1180    +31   
+ Partials        308        300     -8   

Powered by Codecov. Last update 139ab95...648b272

codecov-io commented Oct 5, 2016

Current coverage is 73.95% (diff: 100%)

Merging #1379 into master will decrease coverage by 0.70%

@@             master      #1379   diff @@
==========================================
  Files           187        185     -2   
  Lines          5751       5683    -68   
  Methods           0          0          
  Messages          0          0          
  Branches        864        862     -2   
==========================================
- Hits           4294       4203    -91   
- Misses         1149       1180    +31   
+ Partials        308        300     -8   

Powered by Codecov. Last update 139ab95...648b272

@spencergibb spencergibb changed the title from Fix for issue #1124 to Propagate RequestAttributes across Hystrix Thread boundary Oct 5, 2016

@ryanjbaxter

Make sure your spacing is consistent before and after { and }.

I think the code looks OK, lets see if anyone else on the team has any objections as I am not familiar with the code.

@@ -70,6 +71,11 @@ public HasFeatures hystrixFeature() {
return HasFeatures.namedFeatures(new NamedFeature("Hystrix", HystrixCommandAspect.class));
}
@Bean
public HystrixConcurrencyStrategy hystrixConcurrencyStrategy(){

This comment has been minimized.

@spencergibb

spencergibb Oct 18, 2016

Member

Should this be opt-in (@ConditionalOnProperty)? Since there can only be one HystrixConcurrencyStrategy?

@spencergibb

spencergibb Oct 18, 2016

Member

Should this be opt-in (@ConditionalOnProperty)? Since there can only be one HystrixConcurrencyStrategy?

@Writtscher

This comment has been minimized.

Show comment
Hide comment
@Writtscher

Writtscher Oct 26, 2016

Exactly what I need. But @spencergibb is right. There is always one HystrixConcurrencyStrategy. This means as soon as you need to decorate the Callable you need to provide your own HystrixConcurrencyStrategy which is not doing much but decorates the Callable with another Callable.

IMO it would be nice if there would be a DecoratingHystrixConcurrencyStrategy that has a list of CallableDecoration-Components (just a name, maybe an interface where the call method provides the previous decorated Callable to continue chain). As soon as the wrapCallable method is called the strategy creates a DecoratedCallable that decorates the callable with all existing CallableDecorations.

This way it would be easier to hook into it.

I hope this was understandable 😑. What do you guys think?

Edit: I could provide a pull request as soon as I have time and my approach is fine for you.

Writtscher commented Oct 26, 2016

Exactly what I need. But @spencergibb is right. There is always one HystrixConcurrencyStrategy. This means as soon as you need to decorate the Callable you need to provide your own HystrixConcurrencyStrategy which is not doing much but decorates the Callable with another Callable.

IMO it would be nice if there would be a DecoratingHystrixConcurrencyStrategy that has a list of CallableDecoration-Components (just a name, maybe an interface where the call method provides the previous decorated Callable to continue chain). As soon as the wrapCallable method is called the strategy creates a DecoratedCallable that decorates the callable with all existing CallableDecorations.

This way it would be easier to hook into it.

I hope this was understandable 😑. What do you guys think?

Edit: I could provide a pull request as soon as I have time and my approach is fine for you.

@spencergibb

See opt-in comment

@spencergibb

This comment has been minimized.

Show comment
Hide comment
@spencergibb

spencergibb Jan 10, 2017

Member

ping @taxone on requested changes

Member

spencergibb commented Jan 10, 2017

ping @taxone on requested changes

@spencergibb

This comment has been minimized.

Show comment
Hide comment
@spencergibb

spencergibb Aug 16, 2017

Member

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

Member

spencergibb commented Aug 16, 2017

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment