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

Hook execution sequence issue #327

Closed
wants to merge 3 commits into from

Conversation

krutsko
Copy link

@krutsko krutsko commented Oct 20, 2014

  1. Expected onComplete to be executed in all cases at the very end.
  2. in testExecutionHookRunFailureWithFallback - onComplete executed twice.

Unit test changes demonstrating these issues.

@cloudbees-pull-request-builder

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

…ookFailedOnHystrixBadRequestWithSemaphoreIsolation
@cloudbees-pull-request-builder

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

@benjchristensen
Copy link
Contributor

Thanks for the unit test, I'll look at the second issue. For the first issue though the design has been that onComplete does NOT execute in a failed execution, onError does. If you want something that always fires at the end that would be like an onTerminate which fires in both the onError and onComplete case, but you can already do that since onComplete and onError both exist so you can just put your onTerminate trigger in those 2 hooks.

The docs state:

    /**
     * Invoked after completion of {@link HystrixCommand} execution that results in a response.
     * <p>
     * The response can come either from {@link HystrixCommand#run()} or {@link HystrixCommand#getFallback()}.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     * @param response
     *            from {@link HystrixCommand#run()} or {@link HystrixCommand#getFallback()}.
     * @return T response object that can be modified, decorated, replaced or just returned as a pass-thru.
     * 
     * @since 1.2
     */
    public <T> T onComplete(HystrixCommand<T> commandInstance, T response) {

    /**
     * Invoked after failed completion of {@link HystrixCommand} execution.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     * @param failureType
     *            {@link FailureType} representing the type of failure that occurred.
     *            <p>
     *            See {@link HystrixRuntimeException} for more information.
     * @param e
     *            Exception thrown by {@link HystrixCommand}
     * @return Exception that can be decorated, replaced or just returned as a pass-thru.
     * 
     * @since 1.2
     */
    public <T> Exception onError(HystrixCommand<T> commandInstance, FailureType failureType, Exception e) {

@mattrjacobs
Copy link
Contributor

This was manually merged via #378. Thanks @krutsko for the contribution!

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.

4 participants