Batch API finishes when >99.5% of items have been processed

Ew. This is ridiculous :)

Of course we need tests for this. The fix should work for D7, for D8 we will need to make sure that we don't rely on floating point comparisons to determine if the batch is finished, _batch_process() should report this information explicitly.

Marking as critical because this can break stuff in a lot of fun ways, including the upgrade process.

Bookmarking this to come back to later, but off the top of my head I'm not seeing where the bug is. I tried running this test script:

define('DRUPAL_ROOT', getcwd());
require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
require_once DRUPAL_ROOT . '/includes/batch.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
print _batch_api_percentage(501, 500);

and it gives me "99.8", which is not equal to 100... Where do you see the incorrect rounding happen?

Note that there are existing (extensive) tests for this function in modules/simpletest/tests/batch.test, written by Charlie Gordon. It's possible that this is the most extensively-tested function in all of Drupal, but if it needs more, it needs more :)

Looking at the code, I can definitely see the bug:

In _batch_progress_page_nojs():

    list($percentage, $message) = _batch_process($batch);
    if ($percentage == 100) {
      $new_op = 'finished';
    }

In batch.js:

        if (progress == 100) {
          pb.stopMonitoring();
          window.location = settings.batch.uri + '&op=finished';
        }

The problem, as I described in #3, is that _batch_process() does not return a "I am finished" flag, so the front-ends (JS or non-JS) rely on floating point comparison to assess if the batch is finished or not.

I agree with damien, we shouldn't ever rely on floating point comparison. I saw to many troubles with this on non drupal projects I worked on. Now we need someone who actually can fix _batch_process() ;)

That might be a good idea, but I don't see how it's related to this issue.

Even if there is a flag, we still need the code to set the flag correctly or it won't work very well :) And we always want the percentages to be correct anyway, or otherwise we'd print "100%" on the screen for batches that aren't actually done yet.

Based on @janusman's description in #5, the bug is just that _batch_api_percentage() assumes it will only get integers passed in (for $total and $current), but sometimes it doesn't. So either the assumption or inputs have to be changed.

Adding backport tag.

@janusman :
Batch operations can be multipass. Some ops can therefore be 'halfway through', and the completion level of a given op is therefore a float between 0 and 1. The completion level of the whole batch is therefore a float too. Why would a calculation mixing ints and floats be weird ?

A patch that renames the $finished variable within _batch_process() to something clearer is definitely acceptable, if you feel it's needed. Or possibly just adding a one line comment above the $current = ... line: "Total progress is the number of operations that have fully run plus the completion level of the current operation".

As for the possible revert of _batch_api_percentage(), this would really need some feedback from the folks who introduced it in the first place (boombatower, cwgordon7).

I was also pretty involved in the issue that added _batch_api_percentage(), and I don't see why we would revert it. It was added for a reason (to make sure the percentages printed were always correct and meaningful). Always rounding down doesn't make sense.

As I described in #9, I think the bug is just that when we introduced that function, we made a mistake in thinking it would only ever get integers as input. However, we forgot about multi-step operations, where it turns out you can have a batch whose current state is something like 2.998 out of 3 items completed.

I think the attached patch should fix it, right? It comes with a test that is based directly on @janusman's Apache Solr example from above :)

That additional code comment looks very reasonable to me.

#16: 1089472-16-batch-api.patch queued for re-testing.

Ok it took me an hour to write this post lol. So I thought about this a lot. I tested a lot of cases and if the function gets called with integers it works perfectly . If it get called with floating point values it can get nasty.

Example: (500 steps done out of 501 but in floating point values)
---------

$total = 1;
$current = 0.998003992016; (== 99.8 %)
than it stops to fast. Primarily because

floor(log10($total / 2.0)) - 1

returns some kind of negative value. BUT that negative value gets 0 when we do.

$decimal_places = max(0, floor(log10($total / 2.0)) - 1); 

== 0 in our case.

SO if we were working with integer and we had 500 steps done out of 501, the previous calculation would return 1 an not 0.
If the calcualation returns 0 (like we have now) 99.8% gets rounded to 100% ==> error. Thats why we add one to $decimal_places each time we loop. After one step the number will be 99.8%, the loop will finish and we have a correct value.

This will not keep looping if we are in the final step (501 out of 501), because we check that before we start looping.
The tests are perfect. So this sounds RTBC.

Lets get this in ;)

I also spent about 45 minutes looking at this. I didn't really like the while-loop at first, but it actually looks like the proper fix. Committed to 7.x and 8.x. Thanks!

Removing tag.

Pleaseeave the tags in place. It's the only way to track issues affecting both versions at the moment.