Concurrent ajax submits cause user data loss

Protection against this sounds good, but we don't count a lost form submission as critical per https://www.drupal.org/core/issue-priority. Also is this significantly different from two non-AJAX forms being submitted for say config save - the last one to hit the server will win in that case.

Also it's so, so good that we can have test coverage for concurrent AJAX, nice one!

A queue for ajax requests could be problematic as the response of the first request could alter the DOM in a way that makes the sending of the second one wrong.
For example, if the button that triggers the second request is removed by the response to the first one it would not make sense or even be a logical error to still execute the second request.

Had the same issue, I've implemented what was discussed and amended the failing test

fixed some issue with the previous patch

for those of use who need this in D8

#18 works for me - when ajax request is in progress, pressing on another button does nothing and concurrent ajax request isn't triggered.

I would propose to disable ajax buttons (make them inactive) if there is some ajax request is in progress - it can be unclear for users why they press the button but nothing happens.

I tested manually and it works. I made a code review on the patch as follows:

1. +++ b/core/misc/ajax.es6.js
   @@ -726,6 +737,12 @@
   +    // An ajax callback can creating disrupting changes so
   +    // we don't permit concurrency if its a blocking request.
   

   This should be rephrased.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FormAjaxTest.php
   @@ -0,0 +1,144 @@
   +class FormAjaxTest extends WebDriverTestBase {
   

   Class name could refer to the comment and follow the rest of the test class namings with Ajax like: AjaxFormResponseTest.php

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FormAjaxTest.php
   @@ -0,0 +1,144 @@
   +   * Tests that sending a second ajax request while the first one is still
   +   * running the form will be rebuilt and saved correctly.
   

   Let's simplify this annotation to Tests concurrent ajax form requests and provide more explanation as a comment in the function.

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FormAjaxTest.php
   @@ -0,0 +1,144 @@
   +  public function testAjaxRequestBeforeCompletingPreviousAjaxRequest() {
   

   This could become: testConcurrentAjaxRequests()

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FormAjaxTest.php
   @@ -0,0 +1,144 @@
   +    $web_assert = $this->assertSession();
   

   $web_assert should become $assert_session.

6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FormAjaxTest.php
   @@ -0,0 +1,144 @@
   +    // check that we can't submit the second ajax request.
   

   This comment should start with capital letter.

7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FormAjaxTest.php
   @@ -0,0 +1,144 @@
   +    // Ensure second requests have been processed.
   

   Request should be singular here.

8. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FormAjaxTest.php
   @@ -0,0 +1,144 @@
   +      $page_field_value = $this->getSession()->getPage()->findField($locator)->getValue();
   

   We have the $page variable to use here.

9. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FormAjaxTest.php
   @@ -0,0 +1,144 @@
   +    $this->drupalPostForm(NULL, [], t('Save'));
   

   I think we don't need t() here.

10. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FormAjaxTest.php
    @@ -0,0 +1,144 @@
    +    // Assert that all the fields contain the value that we have set.
    

    This comment is not required.

11. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FormAjaxTest.php
    @@ -0,0 +1,144 @@
    +      $page_field_value = $this->getSession()->getPage()->findField($locator)->getValue();
    

    We have the $page variable to use here.

@joevagyok, @sinn, thank you both for your feedback, I've implemented above. About the suggestion with disabling the buttons, I've thought about it.. for now I would not do it for 2 concerns: once this is merged other elements will be able to call blocking requests, and potentially, not all buttons will be blocking. It can be done but I think for now the visual aids like throbber and the waiting message are sufficient.

1. +++ b/core/misc/ajax.es6.js
   @@ -12,6 +12,13 @@
   +  drupalSettings.ajaxAllowed = true;
   

   I don't think this belongs inside the drupalSettings object. I'd put it inside the Drupal.ajax object, which olds the ajax instances already and some other data.

2. +++ b/core/modules/field/tests/modules/field_plugins_test/src/Plugin/Field/FieldWidget/TestFieldWaitingWidget.php
   @@ -0,0 +1,29 @@
   + *   label = @Translation("Text field waiting"),
   

   The name is little representative of what the field widget is for.
   But anyway, I would remove the current approach altogether, see below.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormResponseTest.php
   @@ -0,0 +1,143 @@
   +class AjaxFormResponseTest extends WebDriverTestBase {
   

   We are not testing the response at all though, we are testing concurrency.

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormResponseTest.php
   @@ -0,0 +1,143 @@
   +  protected static $modules = ['node', 'field_plugins_test'];
   

   We shouldn't write the test depending on node and fields. We should use the already existing AJAX test module to provide a new route with the slow AJAX response behaviour.

The patch works well, I agree with the UX issue of clicking other buttons but nothing happens. I also agree that it's a bit limited by the throbber presence. We can see what UX experts think.

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -544,6 +544,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+        'blocking' => FALSE,

I checked better why we are doing this and I noticed it's needed because an AJAX response triggers additional AJAX requests.
I think the solution should be adding the ajaxAllowed = true also inside the success method, the one defined in the ajax.options. At that point the response has been validated and the various response commands are processed inside Drupal.Ajax.prototype.success. So we can allow subsequent requests to start. The one in the complete method should be kept as it will allow resetting the values after responses with errors.

and one for d8

I reworked the tests to cover more scenarios.
The provided patch here has some tests that are meant to fail to unravel some bugs present in 27.
In #25 I asked to not depend on node for the tests, which I did in this patch but I admit now the tests is a bit more obscure, as it doesn't show the data loss as clearly as using the widget. I could add some visual output of the current counters, but at the same time this is a test so not sure if it's needed.
There's still a scenario that is not covered and I'm not sure how to tackle it. A "slow, non-blocking" request could trigger a subsequent blocking request. If any other requests that change the form have been started during the "slow, non-blocking" request but before the subsequent blocking request, the form would be outdated. I think that the developer should mark the ""slow, non-blocking" trigger element correctly to prevent this scenario.

Actually non-blocking requests should always be allowed, even if other requests are being executed. I refactored the tests and the code to mark this behaviour.
As usual I uploaded the previous interdiff with the wrong extension, so I'm re-uploading it here.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/ConcurrentAjaxRequestsTest.php
@@ -0,0 +1,104 @@
+    $this->clickLink('Non-blocking request');
+    // Wait for the non-blocking request to complete successfully.
+    $this->assertTrue($assert_session->waitForText('Current theme: stark', 2000));

I don't like this part as we are waiting an arbitrary 2 seconds, which could cause random failures. The thing is we need to have the slow request run long enough for this request to be over, to test that non-blocking requests do not reset the js flag of blocking requests as it was happening in #31.

This is not limited to AJAX. I have experienced this via graphQL as well.

I think a better place for any fixes of this sort of thing would be in the Entity API, perhaps some kind of lock like you get in databases where if you read with the intention of saving the result it locks any further reads of the same kind.

We already have EntityChangedConstraint/EntityChangedConstraintValidator in core, however there are some outstanding issues for translations and revisions (#2920889: EntityChangedConstraintValidator doesn't take adding, removing and reverting translations into account and related).

I think the entity system is the better place to continue things like this - even if we added locking to the AJAX system, you can have conflicts between AJAX and non AJAX forms, or web services etc. So adding locking AJAX just at the AJAX level could lead to a false sense of security.

I don't like too much the "locking" thing either, but the issue is present also at form level for non-entity forms.
The test I attached in #33 uses a normal form to prove the bug. So I am not sure that fixing this at entity system level would help the remaining form scenarios.

I'm moving this to a feature request for the forms system.

For entities we need protection at the entity level, since they're not only updated via forms. Other subsystems where this is a problem could also implement validation. Adding it at the form API level might an idea but doesn't really feel like the cause of actual bugs which is dependent on what's being submitted.

Added references to some related issues.

Patch #33 works well for me. It solves my problem.
I reproduced the original issue with the node where I have paragraphs (paragraphs module) with multiple nested paragraph entities and when I clicked (with a very short time delay) "Collapse" and add new item (paragraph entity) buttons. As result, I experienced showing different not related fields.
After applying the patch I cannot do this anymore (trigger other ajax requests), so I don't have such a problem. As I understand it is related to an incorrect update of the form build id.

This is a big issue on content types with a large paragraph structure.
I've updated the patch to work with D9.5

Patch #33 works well for me.

I have attached the patch for d10. Please review

The patch I submitted earlier breaks paragraphs forms. DO NOT USE!
Also, after doing some more tests, I realized the issue has been fixed in the core and there is no more need for the patch.

@bogdan.dinu you said this has been resolved? Do you know where it was fixed? So we can track down the ticket and move over credit for users who helped here.

@smustgrave unfortunately no. I don't know where this was fixed. I just tried locally on D9.4 without the patch and saw that there is no more data loss.

This issue is pretty old, so it might have been #2837022: Concurrently editing two translations of a node may result in data loss for non-translatable fields or a similar issue.

We could also mark it duplicate of #2920251: Offer concurrent editing protection for all entities by including the changed timestamp to each entity form maybe which is still active and covering similar ground.

Will move over to #2920251: Offer concurrent editing protection for all entities by including the changed timestamp to each entity form

This is not fixed, I'm still experiencing this on a Drupal 9.4.9 project with bricks and on a Drupal 9.5.9 project with paragraphs.
And how could this be marked as a duplicate of an issue tied to the entity system component? That's disregarding the comment in #41 about non-entity forms.
I also disagree about the category change. To me, if a form breaks when clicking on a button, it's a bug (sorry if there's something I misunderstood).

Hiding patch per #49.

Ok, I think I see.
The issue's title and summary focus on the backend part of the problem, which should indeed be handled by validation.
But what brought me here is the mess caused by this bug on frontend: when it occurs, the whole paragraph/brick subform gets replaced by other unrelated fields.
What can be lost is not saved data, just contributed content.
Unless I miss something, I don't think this will get fixed with #2920251: Offer concurrent editing protection for all entities by including the changed timestamp to each entity form, will it?
If not, what's the best way to handle this, change this issue's title and summary or create a new one?

Rerolled patch #33.

Here's patch #33 re-rolled without mysterious filename change...

Can the issue summary be updated to include why this is different then the related issue. + remaining tasks, etc.

Also please include any interdiff

Fixed build error and attached interdiff for same. and updated issue summary

Patch needs to be updated for 10.1.x-dev,

Maybe even 11.x-dev at this point.

Re-rolled patch #61 for 10.1.x-dev.
Needs review.

Tried to fix failed custom commands.

We really should target the latest development branch and backport

I have re-rolled the patch #61, for 11.x-dev. please review

Can the issue summary be updated with proposed solution.

"We need to solve this both in FormBuilder" but how is it being solved? Helps the reviewers.

Rerolled patch #64 for 10.2.x, as it doesn't apply there

Uploaded the wrong copy -_-

Rerolled patch #64 for 10.2.x, as it doesn't apply there