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

Throwable in extended fallback is null for timeouts #974

Closed
victormferrara opened this issue Nov 13, 2015 · 14 comments
Labels

Comments

@victormferrara
Copy link

@victormferrara victormferrara commented Nov 13, 2015

Hi,

I've been using the extended fallbacks made in #828 (which are great) and I've seen that when the execution times out the Throwable is null as the execution didn't actually fail. It would be nice to have some kind of TimeoutException there to know the real reason and don't have to dead with nulls. Looking at the code, probably the same thing happens with rejected executions.

Thanks!

@mattrjacobs

This comment has been minimized.

Copy link
Contributor

@mattrjacobs mattrjacobs commented Nov 13, 2015

Is this specific to hystrix-javanica, or are you using Hystrix directly?

@victormferrara

This comment has been minimized.

Copy link
Author

@victormferrara victormferrara commented Nov 16, 2015

hystrix-javanica

@mattrjacobs

This comment has been minimized.

Copy link
Contributor

@mattrjacobs mattrjacobs commented Nov 16, 2015

@dmgcodevil Could you take a look at this one also, please?

@dmgcodevil

This comment has been minimized.

Copy link
Contributor

@dmgcodevil dmgcodevil commented Nov 16, 2015

Yes, I'll take a look
On 16 Nov 2015 12:31 p.m., "Matt Jacobs" notifications@github.com wrote:

@dmgcodevil https://github.com/dmgcodevil Could you take a look at this
one also, please?


Reply to this email directly or view it on GitHub
#974 (comment).

@dmgcodevil

This comment has been minimized.

Copy link
Contributor

@dmgcodevil dmgcodevil commented Nov 17, 2015

@mattrjacobs btw, it largely relates to hystrix core, to get exception javanica uses getFailedExecutionException(), do you have any clue why it returns null in the case of timeout error ?

@mattrjacobs

This comment has been minimized.

Copy link
Contributor

@mattrjacobs mattrjacobs commented Nov 17, 2015

Thanks for taking a look, @dmgcodevil. I'll dive in and see what's happening in hystrix-core.

@mattrjacobs

This comment has been minimized.

Copy link
Contributor

@mattrjacobs mattrjacobs commented Nov 17, 2015

@victormferrara The existing semantics here are fairly subtle. The getFailedExecutionException() only refers to exceptions that arose during the execution of the work in the HystrixCommand / HystrixObservableCommand.

So in the timeout case, the execution of the command never threw a timeout, the surrounding Hystrix machinery did when it timed out that work. Likewise, in a rejection case, the execution of a command never happened, so there is no execution exception to report.

What problem are you trying to solve with this value? I know that there are other ways to query the command (isResponseTimedOut() / isResponseRejected()) that should give you the same information without giving you the actual Exception instance.

@dmgcodevil

This comment has been minimized.

Copy link
Contributor

@dmgcodevil dmgcodevil commented Nov 18, 2015

@mattjacobs, he is complaining because javanica passes null in extended
fallback method that is really annoying and can facilitate bogus code and I
agree with it. So the question is what we can do in order to avoid passing
null in fallback if a command fails because of timeout or rejected. I think
I can use the methods that you suggested and create exception with
corresponding text and pass it to fallback using additional argument. So
your answer helped, I guess
On 17 Nov 2015 4:46 p.m., "Matt Jacobs" notifications@github.com wrote:

@victormferrara https://github.com/victormferrara The existing
semantics here are fairly subtle. The getFailedExecutionException() only
refers to exceptions that arose during the execution of the work in the
HystrixCommand / HystrixObservableCommand.

So in the timeout case, the execution of the command never threw a
timeout, the surrounding Hystrix machinery did when it timed out that work.
Likewise, in a rejection case, the execution of a command never happened,
so there is no execution exception to report.

What problem are you trying to solve with this value? I know that there
are other ways to query the command (isResponseTimedOut() /
isResponseRejected()) that should give you the same information without
giving you the actual Exception instance.


Reply to this email directly or view it on GitHub
#974 (comment).

@victormferrara

This comment has been minimized.

Copy link
Author

@victormferrara victormferrara commented Nov 18, 2015

@mattrjacobs yeah, it's exactly what @dmgcodevil explained. Thanks!

@mattrjacobs

This comment has been minimized.

Copy link
Contributor

@mattrjacobs mattrjacobs commented Nov 19, 2015

hystrix-core itself is generating these Short-Circuit and Timeout exception instances. Are we missing a feature where hystrix-core exposes these exceptions, so javanica does not need to?

@dmgcodevil

This comment has been minimized.

Copy link
Contributor

@dmgcodevil dmgcodevil commented Nov 26, 2015

@mattrjacobs it could be better if we had an abilitty to get such exceptions within a command

@mattrjacobs

This comment has been minimized.

Copy link
Contributor

@mattrjacobs mattrjacobs commented Nov 30, 2015

@dmgcodevil Agreed. I just added #998 to add this to hystrix-core. Once it's there, hystrix-javanica can leverage this. Will note the PR in these comments so you can review

@mattrjacobs

This comment has been minimized.

Copy link
Contributor

@mattrjacobs mattrjacobs commented Dec 3, 2015

The new method in hystrix-core to get this exception is now merged in master via #1008 / #1003

@mattrjacobs

This comment has been minimized.

Copy link
Contributor

@mattrjacobs mattrjacobs commented Apr 4, 2016

This should be fixed in #1167

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.