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

Provide context for runFunc which is cancelled during timeouts #84

Open
cyrusaf opened this issue Feb 20, 2018 · 2 comments
Open

Provide context for runFunc which is cancelled during timeouts #84

cyrusaf opened this issue Feb 20, 2018 · 2 comments

Comments

@cyrusaf
Copy link

cyrusaf commented Feb 20, 2018

Currently, when running hystrix.Do or hystrix.Go, there is no built in context cancellation during a timeout. For example, when making a dynamo query:

err := hystrix.Do(HystrixCommand, func() error {
    results, err := this.ddb.QueryWithContext(ctx, query)
    return err
}, nil)

If hystrix times out here, the dynamo query will not be cancelled (since the context is not cancelled) and may stay open forever. I had a case recently, where I reached my dynamo read capacity and this query stayed open for > 80 seconds.

It is possible to add my own context cancellation:

ctx, cancel := context.WithCancel(ctx)
err := hystrix.Do(HystrixCommand, func() error {
    results, err := this.ddb.QueryWithContext(ctx, query)
    return err
}, func(err error) error {
    cancel()
    return nil
})

But I think it should be built into the library:

err := hystrix.Do(ctx, HystrixCommand, func(ctx context.Context) error {
    results, err := this.ddb.QueryWithContext(ctx, query)
    return err
}, nil)

Where the passed in context to hystrix.Do is wrapped with context.WithCancel(ctx), passed into the runFunc, and then the cancel func is called when there is an error/timeout.

@afex
Copy link
Owner

afex commented Feb 20, 2018

this feature is introduced in #79

please try out that branch and let me know if it fits your needs

@jordanpotter
Copy link

Just took a look at #79 (which I see has been merged). Doesn't appear that the context is canceled when the circuit times out 🤔

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

No branches or pull requests

3 participants