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

Fixed NPE on request cache when HystrixRequestContext is not initialized #1093

Merged
merged 3 commits into from
Feb 8, 2016
Merged

Fixed NPE on request cache when HystrixRequestContext is not initialized #1093

merged 3 commits into from
Feb 8, 2016

Conversation

caarlos0
Copy link
Contributor

@caarlos0 caarlos0 commented Feb 6, 2016

I'm new to Hystrix, so I'm not sure this is the ideal implementation.

This might fix #995 .

@cloudbees-pull-request-builder

NetflixOSS » Hystrix » Hystrix-pull-requests #350 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

NetflixOSS » Hystrix » Hystrix-pull-requests #351 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

NetflixOSS » Hystrix » Hystrix-pull-requests #352 FAILURE
Looks like there's a problem with this pull request

@caarlos0
Copy link
Contributor Author

caarlos0 commented Feb 6, 2016

Apparently there are unstable tests..

image

It also passed in Jenkins and travis... but int the last build, for some reason, these two tests failed:

  • com.netflix.hystrix.HystrixCommandTest#testSemaphorePermitsInUse
  • com.netflix.hystrix.HystrixCommandTest#testRequestCacheWithSlowExecution

Any tips?

@mattrjacobs
Copy link
Contributor

@caarlos0 Thanks for the contribution - looks like a good change to me. One subtlety here is that the exception thrown is synchronous, while the method is returning an Observable<T>. I think that this is appropriate, as it distinguishes an actual error in the command data path (async onError) versus an error in the command setup.

I'll merge this and look at the flaky unit tests.

mattrjacobs added a commit that referenced this pull request Feb 8, 2016
Fixed NPE on request cache when HystrixRequestContext is not initialized
@mattrjacobs mattrjacobs merged commit ec642b0 into Netflix:master Feb 8, 2016
@caarlos0 caarlos0 deleted the request-cache branch February 8, 2016 21:42
@caarlos0
Copy link
Contributor Author

caarlos0 commented Feb 8, 2016

@mattrjacobs Nice to hear, thanks for the great lib, btw 🍻

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.

@CacheResult on @HystrixCommand causes a NPE
3 participants