Currently, in D7 and D8 if a queue worker throws an exception it is uncaught. That means the entire PHP process dies. There is an upside to that: It means the item being processed will not be deleted from the queue and can simply picked up by another worker later, after it expires. That is, however, the only upside and easily outweighed by the whole "PHP breaks" downside. (Which has, incidentally, now cost me and my client several hours of swearing because our queue workers talk to outside systems, which is one of the most likely things to break and throw exceptions.)
It's also dead simple to fix, so I don't know why we haven't. The solution? Catch the exception, log it, don't delete.
The attached patch does so. It does not call releaseItem() on the assumption that if there's an exception you probably want to wait a bit before trying again; if a 3rd party service is down, for instance, you want to give it time to come back up before bombarding it with failing connection attempts. Instead, we simply allow the item to expire and get picked up on its own.
I'm pretty sure the same patch will apply to both D7 and D8, but I'm including a version rolled against D7 anyway for completeness. This should get committed to both versions.
Comment | File | Size | Author |
---|---|---|---|
#13 | 2021933-followup-tests-12.patch | 923 bytes | David_Rothstein |
#10 | interdiff-8-10.txt | 1.52 KB | David_Rothstein |
#10 | 2021933-10.drupal.queue-exception.patch | 2.98 KB | David_Rothstein |
#10 | 2021933-10.drupal.queue-exception-TESTS-ONLY.patch | 2.25 KB | David_Rothstein |
#8 | 2021933-8.drupal.queue-exception.patch | 3.06 KB | joachim |
Comments
Comment #1
lz1irq CreditAttribution: lz1irq commentedApplies cleanly to commit c47f5c70ef97e1288a72d4ded06ad436be863d3c.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedyep, i like this patch, waiting_queue does something like this already.
all we need is a test.
Comment #4
Crell CreditAttribution: Crell commentedTests it is.
IMO drupal_run_cron() is in desperate need of refactor, given that I had to make a whole new test module just for this, but that's for another issue.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedlooks good to me.
nitpick - missing Star Wars/Star Trek quote or reference in error message:
Comment #6
Crell CreditAttribution: Crell commentedI considered that, but in this case it really is the Exception you're looking for. *waves hand*
Comment #7
catchCommitted/pushed to 8.x, thanks!
Looks like this could be backported to 7.x.
Comment #8
joachim CreditAttribution: joachim commentedComment #9
Crell CreditAttribution: Crell commentedBoy that looks familiar... :-)
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commented"testFooBar" is an interesting function name, so I changed it to be the same as the Drupal 8 one :) I also removed the D8-specific stuff from elsewhere in the patch. These are minor changes so I'll commit this as long as it passes/fails as expected.
Comment #11
David_Rothstein CreditAttribution: David_Rothstein commentedWell, the patch passed tests, but so did the version that was supposed to fail. I guess that's good enough for now, especially since this looks like the same test that's in Drupal 8 (we can investigate any problems there first, then backport).
Committed to 7.x - thanks!
Comment #13
David_Rothstein CreditAttribution: David_Rothstein commentedHere's a quick test to see if Drupal 8 has the same problem. If the testbot passes this, then the tests here need work (although this could be moved to a new issue as well).
Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedTypo fix.
Comment #16
David_Rothstein CreditAttribution: David_Rothstein commentedOK, somehow Drupal 8 is detecting the exception when Drupal 7 isn't (could have something to do with how the test system calls cron.php in each case).
But really the tests should be fixed in Drupal 8 too since they should really have an assertion that is expected to fail (rather than just relying on an exception). I.e. if the tests managed to reach this line, they'd actually pass in Drupal 8 regardless of whether the exception was caught (as described in the original issue, the failing item is still in the queue before the patch also):
Comment #17
joachim CreditAttribution: joachim commentedWould something along the same lines as this do the trick:
(from core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockCreationTest.php)
Comment #26
joachim CreditAttribution: joachim commentedThe last comment here is by me, and yet I have no idea what this issue is about any more.
> Catch exceptions from queue workers
I'm pretty sure the queue runner does catch exceptions.
Comment #27
Crell CreditAttribution: Crell at Platform.sh commentedComment #32
quietone CreditAttribution: quietone as a volunteer commentedThis was committed to both Drupal 8 and Drupal 7.
If test rest needed should happen in a follow-up task.
Comment #33
quietone CreditAttribution: quietone as a volunteer commentedChanging version to the version of the Drupal 8 commit.