-
Notifications
You must be signed in to change notification settings - Fork 11.4k
[12.x] Introduce FailOnException
job middleware
#56037
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
Conversation
* @param class-string<\Throwable> ...$exceptions | ||
* @return static | ||
*/ | ||
public static function failureIsNot(...$exceptions): static |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we have both failureIsNot
and failureIs
?
In a project, I want to retry only if the exception is related to the HTTP client, but not on other errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I had a very similar middleware on this particular project, but your implementation is cleaner, and I already refactored it. Thanks again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was writing one this morning for a project I'm working on 😅 Good to see it's useful for more than just me.
Just my 2 cents: I would love if we can use RetryWhen instead. when is used everywhere else and sounds more like a sentence. We then could also implement the RetryUnless version. |
Been kind of going back and forth on the name this morning. It seems like a better name would be that this is a circuit breaker. As it's written now, it may seem that it's extending the number of tries (on the job or queue level). |
I was also thinking about the name, maybe Of course, in that case, the condition should be reversed to: if (call_user_func($this->failIf, $e, $job) === true) {
$job->fail($e);
} But perhaps that communicates better the intent, while still achieving the desired outcome (allow to short circuit retrials on an specfic set of exceptions). |
FailIf may be a little bit wonky too, since technically middleware runs before the job is reached, so it may seem like it would prevent the job from running. We'll see what the maintainer says. In my head, I'm thinking something like ShortCircuitRetriesWhen. Or even just making it an optional method on the job itself. (We still have all those naming problems if it were moved, but just wanted to float that thought out there.) |
I would suggest renaming the class to reflect the main use case we have in mind... something like |
RetryIf
job middlewareFailOnException
job middleware
Went with |
Thanks! Could you send me a docs PR for this? @cosmastech |
Why
I want a job to not retry when a particular exception is thrown. For the life of me, outside of calling
$this->fail()
inside the job itself, I do not see a clean way of doing this. Hence this middleware.Example