[10.x] Throw timeoutException instead of maxAttemptsExceededException when a job times out #46968
Conversation
PrinsFrank
changed the title
|
This will be a huge help. Why can't it go to v10 branch? |
ghost
force-pushed
the
timeout-exception-vs-max-attempts-exception
branch
from
2bd41ee to
b3a1143
Compare
PrinsFrank
changed the title
|
Why did this go into the v10 branch? It's a breaking change... |
|
@SlyDave how did this break your app? |
|
Ah I now see the extremely weird decision to make TimeoutExceededException extend MaxAttemptsExceededException. 🤣 I guess that works to handle the breaking change of introducing a new Exception, but it's hard to see how Timeout extends MaxAttempts, maybe a new base Exception in v11, or just remove the extend, both would be breaking. |
|
I still don't understand how this breaks your app? |
… when a job times out (laravel#46968) * Throw timeoutException instead of maxAttemptsExceededException when a job times out * formatting --------- Co-authored-by: Taylor Otwell <[email protected]>
|
It doesn't, as I noted, because of the TimeoutExceededException extending the MaxAttemptsExceeededException. I incorrectly made the assumption that a new Exception was being added, that would be a breaking change, instead one was added that extends the existing one, which does stop the breaking change from occurring, but is really weird as it's utterly unrelated, and just serves to cause confusion in future. If one captures only MaxAttemptsExceeededException, it will capture TimeoutExceededException as well, which is current behaviour but I would argue is unintended behaviour that shouldn't be preserved. |
Currently, both when a job times out and when the max attempts are exceeded for a job the maxAttemptsExceeded exception is thrown. This makes debugging job timeouts difficult. This PR adds a new exception that is thrown when the SIGALRM signal is fired.