While working on #597484: Use the Queue API to fetch available update data, I discovered that Batch API freaks out if $context['finished'] > 1. Hat tip to chx for asking in IRC "wonder what our little batch does if you set finished to 102%" (paraphrase). Anyway, the answer is "it never terminates". ;) Our Batch API unit tests are woefully insufficient to catch this. All they do is ensure that _batch_api_percentage() returns valid output on valid input. Great. Hurray for unit testing! Testing individual functions is so much more important than seeing if the end-to-end system works -- and it's easier to debug this way!... yeah, right. ;)
Anyway, this isn't just a case of "core shouldn't baby sit broken code". If you're using batches for draining queues, and queues can be populated in other threads, this is a potentially common case. It'd be nice if unexpected input didn't trigger an infinite loop, especially since there's basically 0 performance cost in getting this right.
Comment | File | Size | Author |
---|---|---|---|
#20 | 600836-batch-prevent-infinite.patch | 480 bytes | Dave Reid |
#11 | 600836_11.patch | 16.05 KB | naxoc |
#9 | 600836-timcosgrove-batch-never-finishes.patch | 17.77 KB | timcosgrove |
#6 | batch_large_percentage.patch | 6.92 KB | timcosgrove |
#5 | batch_large_percentage.patch | 7.14 KB | timcosgrove |
Comments
Comment #1
dwwHere's the trivial patch to fix the bug. It's 4:25am here, so I don't have time to try to roll a test. That can wait until post 10/15, anyway...
Comment #2
chx CreditAttribution: chx commentedWell, that's just good. Testing indeed can wait, and often while writing upgrades I was wondering what happens here.
Comment #3
webchickUm. Why can testing wait? This is exactly the kind of edge case that screams for a test.
Comment #4
timcosgrove CreditAttribution: timcosgrove commentedAt 4/22 code sprint. Writing test.
Comment #5
timcosgrove CreditAttribution: timcosgrove commentedRe-rolled patch against HEAD; added test for $context['finished'] > 1 to batch.test.
Comment #6
timcosgrove CreditAttribution: timcosgrove commentedRe-submitting cleaner patch
Comment #7
indytechcook CreditAttribution: indytechcook commentedThe batch test finishes nice and quick. Tests look good.
Comment #8
yched CreditAttribution: yched commentedI don't think this is actually related. This is the no-js equivalent of the == 100 check in progress.js
Powered by Dreditor.
Comment #9
timcosgrove CreditAttribution: timcosgrove commentedre-rolled based on yched's comment; also modified all the paths in hook_menu() for style.
It was noticed that the tests use usleep(). This is necessary because the batch needs time to finish for the test to work correctly. The total amount of time this adds is something on the order of 100ms; both necessary and negligible.
Comment #10
yched CreditAttribution: yched commentedCapitalization (2 occurrences of this comment)
wrong indent :-)
no space at the end of lines
same
Powered by Dreditor.
Comment #11
naxoc CreditAttribution: naxoc commentedReroll with a few style changes. There was some code in color.module in the patch from #9 - I removed that assuming that it got in there by mistake :)
Comment #12
sunThanks, looks good to me.
Comment #13
sun#11: 600836_11.patch queued for re-testing.
Comment #14
sun#11: 600836_11.patch queued for re-testing.
Comment #15
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #19
Dave ReidI know this is a big no-no to re-open a closed issue but I'm frankly surprised it was never discussed that this needed to be backported to Drupal 6, especially considering that I've now encountered this bug with XML sitemap. I double checked and we should apply this to D6 since the node access rebuild op could potentially run in an infinite loop.
Comment #20
Dave ReidRe-rolled for D6.
Comment #21
yched CreditAttribution: yched commentedsure.
Comment #22
Gábor HojtsyLooks simple and good, committed, thanks.