Race condition when generating sub directories for image styles [#3211936]

Problem/Motivation

#3036494: Race condition in ImageStyle::createDerivative() partially fixed race conditions when generating image styles. However it did not fix the problem completely.

Steps to reproduce

A fail due to this can be caused by running \Drupal\Tests\thunder\FunctionalJavascript\ArticleCreationTest on apache or nginx. The test randomly fails with:
Failed to create style directory: public://styles/entity_browser_thumbnail/public/2016-05

Proposed resolution

The problem is that image derivatives use the recursive feature of \Drupal\Core\File\FileSystem::mkdir() so while #3036494: Race condition in ImageStyle::createDerivative() fixed the race when creating the last directory in the path ie 2016-05 - it doesn't for any of the others.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comment	File	Size	Author
#15	3211936-15.patch	3.24 KB	alexpott
#15	9.2.x: PHP 7.3 & MySQL 5.7 Patch Failed to Apply 9.1.x: PHP 7.3 & MySQL 5.7 28,029 pass 8.9.x: PHP 7.3 & MySQL 5.7 28,635 pass, 2 fail PHP 7.3 & MySQL 5.7 28,640 pass PHP 7.3 & MySQL 5.7 28,640 pass
#15	12-15-interdiff.txt	826 bytes	alexpott
#12	3211936-12.patch	3.24 KB	alexpott
#12	9.2.x: PHP 7.3 & MySQL 5.7 28,304 pass
#12	8-12-interdiff.txt	1.34 KB	alexpott
#8	3211936-8.patch	3.24 KB	alexpott
#8	9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass
#8	7-8-interdiff.txt	793 bytes	alexpott
#7	3211936-fail-test-7.patch	2.36 KB	alexpott
#7	9.2.x: PHP 7.3 & MySQL 5.7 28,282 pass, 3 fail
#7	6-7-interdiff.txt	1.19 KB	alexpott
#6	3211936-fail-test-6.patch	2.33 KB	alexpott
#6	9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass
#4	3211936-4.patch	3.09 KB	alexpott
#4	9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass
#4	3-4-interdiff.txt	619 bytes	alexpott
#3	3211936-3.patch	3.03 KB	alexpott
#3	9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass
#3	3211936-3.test-only.patch	2.15 KB	alexpott
#3	9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass
#2	3211936-2.patch	901 bytes	alexpott
#2	9.2.x: PHP 7.3 & MySQL 5.7 28,254 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

3 May 2021 at 10:13

alexpott created an issue. See original summary.

Comment #2

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 3 May 2021 at 10:23

Status:

Active

» Needs review

File	Size
3211936-2.patch	901 bytes
9.2.x: PHP 7.3 & MySQL 5.7 28,254 pass

Comment #3

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 3 May 2021 at 11:58

File	Size
3211936-3.test-only.patch	2.15 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass
3211936-3.patch	3.03 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass

Here's a test that proves that finally we have this race condition fixed! Yay.

Comment #4

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 3 May 2021 at 12:30

File	Size
3-4-interdiff.txt	619 bytes
3211936-4.patch	3.09 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass

Hmmmm.... I don't think pcntl_fork() is available on our CI.... the test failed reliably without the fix for me.

Comment #5

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 3 May 2021 at 12:42

Hmmmm.... nope PHP is compiled with --enable-pcntl see https://git.drupalcode.org/project/drupalci_environments/-/blob/producti... and the test in #4 is not going to fail. Maybe DrupalCI is just too quick to cause this to happen. The test in #3 reliably fails for me.

Comment #6

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 3 May 2021 at 13:40

File	Size
3211936-fail-test-6.patch	2.33 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass

Let's see if waiting a bit and all starting at the same time gets us a fail.

Comment #7

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 3 May 2021 at 13:47

File	Size
6-7-interdiff.txt	1.19 KB
3211936-fail-test-7.patch	2.36 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,282 pass, 3 fail

Let's do this a little bit more elegantly with less looping.

Comment #8

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 3 May 2021 at 14:07

File	Size
7-8-interdiff.txt	793 bytes
3211936-8.patch	3.24 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass

And now let's add the fix back in...

Comment #9

3 May 2021 at 14:47

The last submitted patch, 7: 3211936-fail-test-7.patch, failed testing. View results

Comment #10

4 May 2021 at 17:12

The last submitted patch, 7: 3211936-fail-test-7.patch, failed testing. View results

Comment #11

daffie CreditAttribution: daffie commented 5 May 2021 at 08:28

Status:

Needs review

» Needs work

8 files were hidden/shown/deleted

File	Size
3211936-2.patch	901 bytes
9.2.x: PHP 7.3 & MySQL 5.7 28,254 pass
3211936-3.test-only.patch	2.15 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass
3211936-3.patch	3.03 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass
3-4-interdiff.txt	619 bytes
3211936-4.patch	3.09 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass
3211936-fail-test-6.patch	2.33 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass
6-7-interdiff.txt	1.19 KB
3211936-fail-test-7.patch	2.36 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,282 pass, 3 fail

Patch looks good:

+++ b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php
@@ -196,4 +197,51 @@ public function testDirectoryCreation() {
+    foreach ($directories as $i => $directory) {

The variable $i is not used and can be removed.

+++ b/core/lib/Drupal/Core/File/FileSystem.php
@@ -214,7 +214,10 @@ public function mkdir($uri, $mode = NULL, $recursive = FALSE, $context = NULL) {
+          $success = $this->mkdirCall($recursive_path, $mode, FALSE, $context);
...
+          if (!$success && !file_exists($recursive_path)) {

The variable $succes is not necessary and can be removed or should the if-statement be !$success || !file_exists($recursive_path).

+++ b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php
@@ -196,4 +197,51 @@ public function testDirectoryCreation() {
+    $time_to_start = microtime(TRUE) + 0.1;
...
+        usleep(($time_to_start - microtime(TRUE)) * 1000000);

Can these be replaced with usleep(100000)?

Comment #12

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 5 May 2021 at 08:50

Status:

Needs work

» Needs review

File	Size
8-12-interdiff.txt	1.34 KB
3211936-12.patch	3.24 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,304 pass

Thanks for the review @daffie

#11.1 Fixed
#11.2 This is correct. If $success is TRUE then we will not bother evaluating file_exists($recursive_path) and therefore save I/O.
#11.3 No. This is necessary to ensure that the race condition occurs. Looping and forking takes time that we have to account for. And is documented.

        // Sleep so that all the forks start preparing the directory at the same
        // time.

Comment #13

daffie CreditAttribution: daffie commented 5 May 2021 at 09:07

Status:

Needs review

» Reviewed & tested by the community

2 files were hidden/shown/deleted

File	Size
7-8-interdiff.txt	793 bytes
3211936-8.patch	3.24 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,256 pass

@alexpott: Thank you for the explanations.

There is a fix and testing for the fix.
The test fails on my local machine when I remove the fix.
All code changes look good to me.
For me it is RTBC.

Comment #14

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 9 May 2021 at 23:20

+++ b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php
@@ -196,4 +197,51 @@ public function testDirectoryCreation() {
+   *
+   * Image style generation call result in many calls to create similar
+   * directory paths. This test forks the process to create the same situation.
+   */

Should this be 'can result'?

```
+++ b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php
@@ -196,4 +197,51 @@ public function testDirectoryCreation() {
+    }
```
One the one hand it's great we can test this, on the other I'm a bit concerned about test stability (and maintainability) with this one. But seems worth a go.

Comment #15

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 10 May 2021 at 10:03

File	Size
12-15-interdiff.txt	826 bytes
3211936-15.patch	3.24 KB
9.2.x: PHP 7.3 & MySQL 5.7 Patch Failed to Apply 9.1.x: PHP 7.3 & MySQL 5.7 28,029 pass 8.9.x: PHP 7.3 & MySQL 5.7 28,635 pass, 2 fail PHP 7.3 & MySQL 5.7 28,640 pass PHP 7.3 & MySQL 5.7 28,640 pass

File

Size

12-15-interdiff.txt

826 bytes

3211936-15.patch

3.24 KB

9.2.x:
PHP 7.3 & MySQL 5.7 Patch Failed to Apply

9.1.x:
PHP 7.3 & MySQL 5.7 28,029 pass

Thanks for the review @catch.

Fixed #14.1
Re #14.2 - in some ways this test is a good thing if we to move Drupal to a place where we support event-loop PHP frameworks like react-php. It'll keep us a bit honest about what happens in destructors. I think the benefits of have test coverage here outweigh maintainability issues:

we skip the test if the environment doesn't support forking
the single work around is not that bad
the test fails reliably without the fix on CI and locally for me

Comment #16

11 May 2021 at 21:15

catch committed 2690c63 on 9.3.x

Issue #3211936 by alexpott, daffie: Race condition when generating sub...

Comment #17

11 May 2021 at 21:15

catch committed 2f5eb02 on 9.2.x

Issue #3211936 by alexpott, daffie: Race condition when generating sub...

Comment #18

11 May 2021 at 21:15

catch committed 220c6c9 on 9.1.x

Issue #3211936 by alexpott, daffie: Race condition when generating sub...

Comment #19

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 11 May 2021 at 21:16

Title:	Race condition when generating sub directories for image styles	» [backport] Race condition when generating sub directories for image styles
Version:	9.2.x-dev	» 8.9.x-dev

Committed/pushed to 9.3.x and cherry-picked back to 9.1.x

This can probably go into 8.9.x but the test failed, so asking for a re-run.

Comment #20

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 14 June 2021 at 07:02

Title:	[backport] Race condition when generating sub directories for image styles	» Race condition when generating sub directories for image styles
Version:	8.9.x-dev	» 9.1.x-dev
Status:	Reviewed & tested by the community	» Fixed