Adding a new batch set while the batch is running breaks batch order

I removed extraneous tags.

Related, https://github.com/drush-ops/drush/issues/1930

#2664290: Remove array typehints from batch callbacks will be the fix I think.

Confirming the bug.

However installing a module with an update is more suitable to be done in hook_post_update_NAME instead in hook_update_N, but atm the problem occurs with both hooks.

Let's mark this as duplicate of #2664290: Remove array typehints from batch callbacks then

I can reproduce this issue in 8.1.7.
It seems that the local.batch.inc file is not included in some cases. Shouldn't we have a module_load_include('batch.inc', 'locale'); call right in _locale_translation_batch_status_operations()?

Here is a quick patch, just in case it helps someone.

I confirm that the problem still exists.

Just conceptually #12 totally makes sense. When a function needs some external code, it should declare that as dependency by loading it.

This looks testable.

I tried to figure out how to test it but I don't know.
Any help, please?

I'd still need some directions to start working on the tests here. Could anyone help me, please?

I agree with #2664016-14: Adding a new batch set while the batch is running breaks batch order and I couldn't think of any suitable test, therefore putting it back again to RTBC.
I hope this could be accepted as it is without tests as it is an obvious bug and bug fix.

I am sorry if it is not possible to accept it without tests and feel free to put it back to "needs work".

How about adding a test where we have an update function that installs a module - we have update path testing. Seems possible to me. Look at the things that extend \Drupal\system\Tests\Update\UpdatePathTestBase - specifically one like \Drupal\system\Tests\Update\UpdatePostUpdateFailingTest would give you hints about how to install a test module prior to running updates - and then this test module should have an hook_update_N() and post update function that both install different modules.

Here's a reroll (the new array syntax broke the patch)

Nothing new besides that, works for me.

As we've just ran into this issue again I've started looking more deeper and searching for the exact problem. I've came to the conclusion, that the problem relies in the batch API when setting/adding a new batch(es) while a one is being processed. What happens is the following:

1. Define 3 batches with operations.
2. batch_set($batch_0);
3. batch_process();
4. During the processing of the first batch two new batches are being added through batch_set.
5. Because there is an active batch the new batch being added will be not placed at the end but prepended to the batch set stack right after the current active batch, but the operations of the other batches, that are already in the db queue will not be reordered. The batch set list will look now like this - $batch_0, $batch_1, $batch_2, but the queued operations will look like this - drupal_batch:49:0 (batch 0), drupal_batch:49:1 (batch 1), drupal_batch:49:1 (batch 2). This causes a big problem where the batch sets are being reordered, but their corresponding operations remain with the initial indexes used for the batch sets and each new batch added while a current one is being processed will have its operations queued with the same index.

I was looking for a solution where we can prepend batches, but I am really sure we need that a complex solution. It has not worked yet properly therefore I think it will not be an API break to just define the batches so that a new batch will always be appended to the stack no matter if one is being currently processed or not.

I am even tempted to raise the issue status to critical, as this might under some conditions break systems, but only raising it to major.

I think that this problem exists since the queue for the batch operations has been introduced in D7 in #629794: Scaling issues with batch API.

After thinking over it again I understand the requirement that when batches are set during an active batch that those new batches are processed before the existing ones in the queue. The problem is that currently this is broken and breaks the order of the queued operations. It also currently works like this that if we set two batches during an active batch the second new one will be placed before the first new one. This is not correct as well. The new patch I am uploading should take care of that. The ProcessingTest should be failing, which I will fix in the next patch version.

Yeah, while I'm not yet sure why we do this dance, this certainly is a bug.

Haven't looked at the patch in #27 yet, but #25 is not quite correct. ;-) Unless I'm mistaken the bug has been introduced in the very first Batch API patch, in fact I traced it back all the way back to #56 in #127539: batch - progressive operations (à la update.php).

I think #27 is the way to go. I've just fixed one of the failing tests, but still have problems with batch_4 in \Drupal\Tests\system\Functional\Batch\ProcessingTest::testBatchForm().

I've found the reason for the failing test and it was that with the new implementation of batch_set I was destroying the reference to the current batch set and it was impossible to update it if during a batch operation batch_set() has been called.

Extending the comment to explain the other problem that the patch is solving, which was present as well.

The test coverage has been updated :).

Just a little code refactoring.

Updating the issue summary to help better understand the problems we have.

Adding additional test coverage as requested so by @dawehner in IRC.

And here the test only patch and the full patch. This bug exists in 8.4.x and in 8.3.x, luckily the patch applies to both branches.

P.S. is backport for Drupal 7 needed as well?

After I've woke up today I realized that the current patch has introduced a bug. We are not maintaining ordered numeric keys anymore and it might happen that the keys are like 0, 3, 4, 1, 2 in which case _batch_next_set() will not work correctly, because it is incrementing the index of the current set by 1, but instead we have to get the next key. We need one more test for such a complex case and a fix for this problem.

Thanks to #40 I've discovered one more problem in the new approach for batch_set() and fixed it as well. I've added additional test coverage for the complex use case :

1. batch_set($batch_4); A batch 4 operation will set $batch_2.
2. batch_set($batch_7); A batch 7 operation will set $batch_5 and $batch_6
3. batch_process();

where the batches should be processed in the following order -

1. batch 4
2. batch 2
3. batch 7
4. batch 5
5. batch 6

+++ b/core/modules/system/tests/modules/batch_test/batch_test.module
@@ -187,13 +247,15 @@ function _batch_test_title_callback() {
+  $state->resetCache();
+  $stack = $state->get('batch_test.stack');
...
+  $state->set('batch_test.stack', $stack);

I am not really sure why, but I had to reset the static cache of the state service before updating the "test batch stack" as otherwise I was observing some race conditions and the test was passing 1 of 4-5 runs only.

It's been a while, but I just stumbled over this and had a look at the patch and - while it took me a while to (re-)grok it - it looks absolutely solid. Neither the runtime code nor the test code is particularly easy to read, but when looking at it in context you see that that's really not the fault of the patch. People think render arrays are the Arrays Of Doom (TM), but I'm not sure whether Batch API really isn't worse.

In any case, thanks!

Also resetting to 8.4.x since this is a bugfix.

+++ b/core/includes/batch.inc
@@ -378,8 +378,10 @@ function &_batch_current_set() {
-  if (isset($batch['sets'][$batch['current_set'] + 1])) {
-    $batch['current_set']++;
+  $set_indexes = array_keys($batch['sets']);
+  $current_set_index_key = array_search($batch['current_set'], $set_indexes);
+  if (isset($set_indexes[$current_set_index_key + 1])) {
+    $batch['current_set'] = $set_indexes[$current_set_index_key + 1];

anyone else think this would be super simplified if it used a proper data structure, e.g. something modeled on \SplStack but that had support for insertBefore/insertAfter?

worth a follow up to experiment if we can replace this with that?

Adding review credits

• @Daniele Nicastro for issue summary updates
• @alexpott for testing guidance

@larowlan Could this help? #66183: Add helper methods to inject items into a particular position in associative arrays

yes, but arrays really aren't data structures...but pretty sure that horse has bolted in Drupal :-P

@larowlan: I had the same thought when looking at this. Since inserting things at particular places in arrays is a common problem in Drupal (e.g. #process, hook_module_implements_alter(), ...) I'm not sure if a generic data structure or a batch-specific object would be best, but in any case let's have a follow-up for that. ++

There's #2401797: Introduce a batch builder class to make the batch API easier to use to try to make batches a bit easier to work with.

There's also #1797526: Make batch a wrapper around the queue system.

This still needs to go into 8.5.x first.

1. +++ b/core/includes/form.inc
   @@ -757,12 +757,54 @@ function batch_set($batch_definition) {
   +      $append_after_index = $batch['current_set'];
   

   Can we move this new code block to an internal helper function to improve readability? Something like _batch_append_set().

2. +++ b/core/includes/form.inc
   @@ -757,12 +757,54 @@ function batch_set($batch_definition) {
   +      // Iterate by reference over the existing batch sets and assign them by
   +      // reference in the new batch sets array in order not to break a retrieved
   +      // reference to the current set. Among other places a reference to the
   +      // current set is being retrieved in _batch_process(). We have to preserve
   +      // the original indexes as they are used to generate the queue name of
   +      // each batch set, as if we do not preserve the indexes of the existing
   +      // batch sets the operations of the new batch will be queued in the queue
   +      // of a previous batch set, because without index preservation we will be
   +      // using not a new, but an existing and already used index for the new
   +      // batch set.
   

   I had an hard time parsing this comment the first time I read it for two reasons:

   • I didn't immediately realize it describes two different bits of logic;
   • the last part of the second paragraph is a bit convoluted and redundant, at least if I understand it correctly.

   What about the following simplified version?

         // Iterate by reference over the existing batch sets and assign them by
         // reference in the new batch sets array in order not to break a retrieved
         // reference to the current set. Among other places a reference to the
         // current set is being retrieved in _batch_process(). Additionally, we
         // have to preserve the original indexes, as they are used to generate the
         // queue name of each batch set, otherwise the operations of the new batch
         // set will be queued in the queue of a previous batch set.
         // @see _batch_populate_queue().
   

3. +++ b/core/modules/system/tests/modules/batch_test/batch_test.callbacks.inc
   @@ -77,15 +77,50 @@ function _batch_test_callback_5($id, $sleep, &$context) {
   +  // No-op, but ensure the batch take a couple iterations.
   

   Typo: takes
   (I know this was copy/pasted from existing code, but let's not add new typos ;)

4. +++ b/core/modules/system/tests/modules/batch_test/batch_test.module
   @@ -149,6 +154,61 @@ function _batch_test_batch_5() {
   + * Batch 6: Performs two batches within a batch.
   

   Shouldn't this be Batch 7?

5. +++ b/core/modules/system/tests/modules/batch_test/batch_test.module
   @@ -149,6 +154,61 @@ function _batch_test_batch_5() {
   +  for ($i = 1; $i <= round($total / 2); $i++) {
   ...
   +  for ($i = round($total / 2) + 1; $i <= $total; $i++) {
   

   Why are we using round here? It seems unneeded and made me wonder whether $total could take other values aside from 10, which does not seem to be the case. Removing round would improve readability IMO.

6. +++ b/core/modules/system/tests/modules/batch_test/batch_test.module
   @@ -187,13 +247,15 @@ function _batch_test_title_callback() {
   +  $state->resetCache();
   

   It's unfortunate we have to this without knowing the reason, we may be covering an existing bug this way. It would be great to add a @todo and a follow-up to investigate this.

Oh, I almost forgot: nice detective work! :)

I can confirm this issue still exists. Just ran into it with the latest webform update under D8.4.4:
drupal/webform (5.0.0-rc2 => 5.0.0-rc3)

[notice] Update started: webform_update_8105
[ok] Update completed: webform_update_8105
[notice] Update started: webform_update_8106
[ok] Update completed: webform_update_8106
[warning] call_user_func_array() expects parameter 1 to be a valid callback, function 'locale_translation_batch_status_check' not found or invalid function name batch.inc:235
[warning] call_user_func_array() expects parameter 1 to be a valid callback, function 'locale_translation_batch_fetch_download' not found or invalid function name batch.inc:235
[warning] call_user_func_array() expects parameter 1 to be a valid callback, function 'locale_translation_batch_fetch_import' not found or invalid function name batch.inc:235
[success] Finished performing updates.

This is still around (D8.5.1):

Performing media_entity_update_8201                                                                                                                                                                    [ok]
call_user_func_array() expects parameter 1 to be a valid callback, function 'locale_translation_batch_status_check' not found or invalid function name batch.inc:163                                   [warning]
call_user_func_array() expects parameter 1 to be a valid callback, function 'locale_translation_batch_status_check' not found or invalid function name batch.inc:163                                   [warning]
call_user_func_array() expects parameter 1 to be a valid callback, function 'locale_translation_batch_fetch_download' not found or invalid function name batch.inc:163                                 [warning]
call_user_func_array() expects parameter 1 to be a valid callback, function 'locale_translation_batch_fetch_import' not found or invalid function name batch.inc:163                                   [warning]
call_user_func_array() expects parameter 1 to be a valid callback, function 'locale_translation_batch_fetch_download' not found or invalid function name batch.inc:163                                 [warning]
call_user_func_array() expects parameter 1 to be a valid callback, function 'locale_translation_batch_fetch_import' not found or invalid function name batch.inc:163      

Addressing #53:
1-5: done.

6:

+++ b/core/modules/system/tests/modules/batch_test/batch_test.module
@@ -187,13 +247,15 @@ function _batch_test_title_callback() {
+  $state->resetCache();

It's unfortunate we have to this without knowing the reason, we may be covering an existing bug this way. It would be great to add a @todo and a follow-up to investigate this.

In #41 I've commented about this:

I am not really sure why, but I had to reset the static cache of the state service before updating the "test batch stack" as otherwise I was observing some race conditions and the test was passing 1 of 4-5 runs only.

Now I've run all the tests from \Drupal\Tests\system\Functional\Batch\ProcessingTest multiple times in a row and no one failed. If there was a bug then it looks like it is not present anymore, therefore I've removed the line reseting the static cache of the state service.

@plach, thank you for the review :)

Checked that the interdiff adresses the last review. Thanks! Back to RTBC now.

It looks like it was just a random failure => back to RTBC.

1. +++ b/core/includes/form.inc
   @@ -755,18 +755,70 @@ function batch_set($batch_definition) {
   + * Appends a batch set to a running batch.
   + *
   + * Inserts the new set right after the current one to ensure execution order,
   + * and store its operations in a queue. If multiple sets are being added while
   + * the batch is running ensure that each new one will be appended instead of
   + * prepended.
   + *
   + * @param &$batch
   + *   The batch array.
   + * @param $batch_set
   + *   The batch set.
   + */
   +function _batch_append_set(&$batch, $batch_set) {
   

   I think this comment could be improved. I don't think we should mention prepend at all. It just confuses things and docs old behaviour and not the current.

   A start on a clearer comment might be:

   Inserts the new set right after the current one to ensure execution order, and store its operations in a queue. If the current batch has already inserted a new set, additional sets will be inserted after the last inserted set.
   

   I think this needs work but it still an improvement.

2. +++ b/core/modules/system/tests/src/Functional/Batch/ProcessingTest.php
   @@ -84,6 +84,31 @@ public function testBatchForm() {
   +    // Batches 4 and 7 with nested batch 2 from within batch 4 and nested
   +    // batches 5 and 6 from within batch 7.
   

   I'm not sure this comment is correct. Well it might be correct but it does not read easy. I would suggest using sentences. I.e.
   Submit batches 4 and 7. Batch 4 will trigger batch 2. Batch 7 will trigger batches 5 and 6.

   Also I think the test would be better if the batch 7 triggered 5 and 6 in the opposite order to prove the order in which the are added takes precedence over anything to do with ordering.

I was thinking about this some more, in particular the array splice work we're doing to keep the order of the batch_set/batch_get tasks and was thinking that perhaps we should be using a data structure better than an array.

E.g. \SplStack gives us LIFO support, or \SplQueue if we need FIFO.

We're basically trying to mimic those data structures with an array, we should try and leverage existing things?

Re #64: I don't think \SplStack or \SplQueue, because we need to be able to insert elements after a specific other elements, while also retaining the information after which an element was originally added. It might be possible to concoct a data structure that would actually have that feature while still allowing simple iteration, but it seems that would be a larger task and I would request that we discuss that in a follow-up, as we might have the need for a similar data structure outside of the Batch system as well, so I could easily see something like that leading to longer discussions. And this is a very tricky and nasty bug, so I would like to get this in as soon as possible.

To emphasize the last point, I recently hit this with the drush locale-update command, where locale adds batch operations for each project inside of another batch. Due to this bug the translations did not end up getting imported because the order of operations was all out of whack.

Still needs work for #63, though.

I m getting same error in 8.5.

I had this error in 8.6.15 and the patch did resolve this issue for me.
Many thanks for the work!

Closed #3059068: Locale batch issues when installing modules via update_N as a duplicate issue.

Same error on 8.7.4 and the patch resolves the issue for me.

Thanks !

any news on that?

#63 addressed.

+++ b/core/includes/form.inc
@@ -775,18 +775,70 @@ function batch_set($batch_definition) {
+ * Inserts the new set right after the current one to ensure execution order,
+ * and store its operations in a queue. If the current batch has already
+ * inserted a new set, additional sets will be inserted after the last inserted

store -> stores

Other than that I think that #63 has been addressed.

#75.

Committed and pushed bb6797a8e2 to 9.0.x and 3b32db50d1 to 8.9.x. Thanks!

Discussed with @catch who +1'd the backport.