[13.x] An exception can implement ShouldntRetry to prevent retries of jobs#60504
[13.x] An exception can implement ShouldntRetry to prevent retries of jobs#60504alexbowers wants to merge 1 commit into
Conversation
|
This makes it more complicated in my opinion |
|
This doesn’t remove the other option, but it adds an alternative that makes code significantly cleaner if you have large jobs that have multiple functions within them.
This also follows a similar process to the ShouldntReport which is used to prevent an exception from being reported
…________________________________
From: gutentagbomb ***@***.***>
Sent: Saturday, 13 June 2026 14:04:06
To: laravel/framework ***@***.***>
Cc: Alex Bowers ***@***.***>; Author ***@***.***>
Subject: Re: [laravel/framework] [13.x] An exception can implement ShouldntRetry to prevent retries of jobs (PR #60504)
[https://avatars.githubusercontent.com/u/120608174?s=20&v=4]gutentagbomb left a comment (laravel/framework#60504)<#60504 (comment)>
This makes it more complicated in my opinion
—
Reply to this email directly, view it on GitHub<#60504?email_source=notifications&email_token=AAGNZXV2PG3HYIHH4SXWJ4L47VGMNA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRZHA2TSOBRGYZKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4698598162>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAGNZXWGZMTUYAS7XDE6SBD47VGMNAVCNFSNUABDKJSXA33TNF2G64TZHM3TKNBYHE4DMO2JONZXKZJ3GQ3DKNBYGE4DGOBVUF3AE>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Where the benefit for this comes in is really when you have a job that extracts its logic into its own methods. Take for example: <?php
class BookFlight implements ShouldQueue
{
use Dispatchable, InteractsWithQueue, Queueable;
public int $tries = 5;
public function __construct(
public Flight $flight,
public Customer $customer,
public array $seats,
public float $amount
) {
}
public function handle(PaymentGateway $gateway): void
{
$reservation = $this->reserve($this->flight);
$charge = $this->charge($reservation, $this->amount);
$invoice = $this->invoice($charge);
$this->book($reservation, $invoice);
}
private function reserve(Flight $flight, Customer $customer, array $seats): Reservation
{
if (! $flight) {
throw new FlightNotFoundException();
// ^^ No point retrying
}
if ($customer->hasAlreadyReserved($flight, $seats)) {
return $customer->getReservation($flight, $seats);
}
if (!$flight->isSeatAvailable($this->seats)) {
throw new SeatNotAvailableException();
// ^^ No point retrying
}
if ($customer->isAllowedSeat($flight, $this->seats)) {
throw new CustomerNotAllowedSeatException();
// ^^ No point retrying
}
return $flight->reserve($this->customerId, $this->seats);
}
private function charge(Reservation $reservation, float $amount): Charge
{
$gateway = app(PaymentGateway::class);
return $gateway->charge($reservation->customer, $this->amount);
// ^^ This can throw for example:
// - PaymentGatewayException (retryable)
// - PaymentTimeoutException (retryable)
// - PaymentNetworkException (retryable)
// - PaymentUnknownException (retryable)
// - PaymentDeclinedException (not retryable)
// - PaymentFraudException (not retryable)
// - PaymentInvalidException (not retryable)
}
private function invoice(Charge $charge): Invoice
{
$invoice = app(InvoiceService::class)->create($charge);
// ^^ This can throw for example:
// - InvoiceInvalidException (retryable)
// - NetworkTimeoutException (retryable)
// - InvoiceAlreadyExistsException (not retryable)
// - InvalidChargeException (not retryable)
$invoice->send();
// ^^ This can throw for example:
// - NetworkTimeoutException (retryable)
// - InvalidEmailException (not retryable)
return $invoice;
}
}To do this without unnecessary retries on the ones that are not worth it, it would look something like this: <?php
class BookFlight implements ShouldQueue
{
use Dispatchable, InteractsWithQueue, Queueable;
public int $tries = 5;
public function __construct(
public Flight $flight,
public Customer $customer,
public array $seats,
public float $amount
) {
}
public function handle(PaymentGateway $gateway): void
{
$reservation = $this->reserve($this->flight);
if (! $reservation) {
return;
}
$charge = $this->charge($reservation, $this->amount);
if (! $charge) {
return;
}
$invoice = $this->invoice($charge);
if (! $invoice) {
return;
}
$this->book($reservation, $invoice);
}
private function reserve(Flight $flight, Customer $customer, array $seats): ?Reservation
{
if (! $flight) {
$this->fail(new FlightNotFoundException());
report($e);
return null;
}
if ($customer->hasAlreadyReserved($flight, $seats)) {
return $customer->getReservation($flight, $seats);
}
if (!$flight->isSeatAvailable($this->seats)) {
$this->fail(new SeatNotAvailableException());
report($e);
return null;
}
if ($customer->isAllowedSeat($flight, $this->seats)) {
$this->fail(new CustomerNotAllowedSeatException());
report($e);
return null;
}
return $flight->reserve($this->customerId, $this->seats);
}
private function charge(Reservation $reservation, float $amount): ?Charge
{
$gateway = app(PaymentGateway::class);
try {
return $gateway->charge($reservation->customer, $this->amount);
} catch (PaymentException $e) {
if ($e instanceof PaymentDeclinedException || $e instanceof PaymentFraudException || $e instanceof PaymentInvalidException) {
$this->fail($e);
report($e);
return null;
}
throw $e;
}
}
private function invoice(Charge $charge): ?Invoice
{
try {
$invoice = app(InvoiceService::class)->create($charge);
$invoice->send();
} catch (InvoiceException $e) {
if ($e instanceof InvoiceAlreadyExistsException || $e instanceof InvalidChargeException || $e instanceof InvalidEmailException) {
$this->fail($e);
report($e);
return null;
}
throw $e;
}
return $invoice;
}
}This approach has various downsides:
With this change, the original example would just work as you'd expect it to, no unnecessary retries, no change of it being missed anywhere because its defined once on the exception instead of per-job, and it falls down to using the default exception handler if necessary too. |
|
How would you add this interface to vendor generated exceptions that are unrecoverable, like the Stripe example in your original description? |
|
My plan would be to instead capture the vendored ones inside of my service code itself, and re-bubble it up with my own exception that extends theirs. Something along the lines of: class MyNonRetryableException extends ExampleVendoredException implements ShouldntRetry
{
}You're right that this wouldn't help with the fully vendored ones that bubble up through, although I am not sure there is anything we can do about that, since there is no form of monkeypatching in PHP that would allow us to inject an interface onto a class at runtime right? |
|
An alternative suggestion to handle this could be something like the That's an alternative implementation in general though, but if you favour that instead, I can take a look at building it and seeing what it would look like. |
Some jobs when processing are useful to be retried a few times, for example, if you hit a rate limit or a temporary network problem, however, within those jobs there can also be times where the system is not recoverable at all.
Currently, you have to call
$this->fail()on these jobs, and it adds a layer of boilerplate that you have to remember for each job you're writing, since it's controlled within the job itself.However, some exceptions are always unrecoverable, for example, in stripe a PaymentDeclined cannot be recovered, and retrying is completely pointless.
Adding
ShouldntRetryon that exception will now make it so that that specific exception flags the job as failed, without the manual boilerplate on each job.To get the same behaviour today, the code looks something like this:
However, this setup gets worse if you have the
$gateway->charge()inside of a method you want to return a value for.Since you cannot just throw an exception without getting the retries, you have to either catch the exception like this, or you can do a
$this->fail()inside that method, but then the return type has to be nullable so that you can return early and handle the response.