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

Allow cancel without retry #988

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

thedmdim
Copy link

Previously we couldn't cancel task without sending it to retry queue. Despite the fact that handler returned SkipRetry error, it didn't work correctly. Read from <-ctx.Done() in processor.go:246 caused by canelation were and will be always earlier than returned SkipRetry from handler. That's why task always went to retry queue.

With these changes you are able to cancel task so it will be archived instead of going to retry :)

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.49%. Comparing base (4f00f52) to head (35edb02).
Report is 17 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #988      +/-   ##
==========================================
- Coverage   67.13%   66.49%   -0.65%     
==========================================
  Files          29       29              
  Lines        4300     5745    +1445     
==========================================
+ Hits         2887     3820     +933     
- Misses       1135     1643     +508     
- Partials      278      282       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kamikazechaser
Copy link
Collaborator

The downside of this approach is that every context cancellation will result in the task being cancelled.. There are some cases where you'd want a certain context cancellation to be retry-able. Right now every cancellation will be retried and I believe most users are used to this behaviour by now.

With 1.20, this kind of checking is possible: https://pkg.go.dev/context#WithCancelCause coupled with errors.Is(context.Cause(ctx), ErrMyRetryableCancelledCtx). This will allow a better experience.

I'd like to keep this PR open and have more discussions around it with more people before we agree on the best way to handle this.

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.

2 participants