When running random test using TestDatabase class under Windows it gets stuck in an infinite loop because of the following line:

web/core/lib/Drupal/Core/Test/TestDatabase.php:118

     if (@symlink(__FILE__, $this->getLockFile($lock_id)) === FALSE) {
        $lock_id = NULL;
      }

Can be seen here as well: https://github.com/drupal/drupal/commit/649e2e68fc1e745f2e8f37530a8264c3...

Under Windows, you cannot create symlinks so simply and since that part of the code is unable to create symlinks, it gets stuck in infinite loop.

Only in version 8.2.0-rc2.

CommentFileSizeAuthor
#18 16-17-interdiff.txt693 bytesalexpott
#17 2804675-2-17.patch4.19 KBalexpott
#16 2804675-2-16.patch4.11 KBalexpott
#10 2804675-10.patch665 byteshideaway
#6 2804675-6.patch960 bytesalexpott

Comments

hideaway created an issue. See original summary.

hideaway’s picture

hideaway commented
Issue summary: View changes
cilefen’s picture

cilefen commented
Title: Tests using TestDatabase class are stucked in an infinite loop in Windows (8.2.0-rc2 only) » Tests using TestDatabase class are stuck in an infinite loop in Windows (8.2.0-rc2 only)
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +8.2.0 release notes

We will need to include this in the release notes for the next release one way or another (whether it's to flag a regression or that this was fixed since RC2).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

According to the PHP documentation it says:

Note: Windows users should note that this function will only work if the system you run PHP from is Windows Vista/Windows Server 2008 or newer. Windows versions prior to that do not support symbolic links.

(https://secure.php.net/manual/en/function.symlink.php)

@hideaway which version of Windows did this fail on?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2804675-6.patch960 bytes

I wonder if there the temp directory is not writable - @hideaway can you run this patch on your environment?

daffie’s picture

daffie commented

The solution looks good to me. By throwing an exception for there not being a working temporary directory you are not getting to the infinite loop. All the code in the patch is good enough to be committed. Normally I would give this patch RTBC, but I do not have a Windows system to test this patch. So a big +1 from me.

hideaway’s picture

hideaway commented

Hi there.

Patch in #6 did not help and I explain why. In Windows in order to create symlinks you need to have "SeCreateSymbolicLinkPrivilege" privilege, which by default is held only by admin. Developers in our office are using Windows 10 Home and they do not have this privilege (and I really did not find a way how to add this in Win 10 Home). And since they cannot run phpunit command with admin rights or run their IDE (PHPStorm) with admin rights to run the tests from there, their migration tests got stuck in an infinite loop. If I run phpunit command with Admin privilege, of course then symlink is created, however since they cannot do this themselves, I currently patched that line of code to use @copy instead, which at least works for us and our migration tests are not stuck anymore.

daffie’s picture

daffie commented

@hideaway: Could you post the patch or the code changes that you made.

hideaway’s picture

hideaway commented
StatusFileSize
new 2804675-10.patch665 bytes

@daffie, of course I can. It was most simple change I could do in order to solve the problem.

daffie’s picture

daffie commented

@hideway: Maybe a stupid question, but do the migration tests now run or are they skipped?

hideaway’s picture

hideaway commented

They definitely run now :) To check I did the following:
1. I checked I have no test* tables in db
2. I run single test migration (like MigrateNodeTest)
3. Exited the test before db cleanup
4. Can clearly see the test* tables with the data.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

With the explanation of @hideaway am I confident enough to give this patch a RTBC status.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Copy isn't atomic, so this could re-introduce the previous race condition.

We could possibly do something with rename()?

Whatever we do, it's going to need an explanatory comment.

fabianx’s picture

fabianx commented

rename() is the only other atomic filesystem operation.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2804675-2-16.patch4.11 KB

Here's another idea that will fix windows and probably a few other tricky situations.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2804675-2-17.patch4.19 KB

A better comment.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 16-17-interdiff.txt693 bytes
fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

First I didn't like that, but now I think this is a good solution.

Especially for unit tests a concurrency of more than one is just very seldom needed.

I think the patch is fine for both 8.3.x and 8.2.x, but probably needs a small change record or add to the existing yet to be created change record.

  • catch committed 1975fe0 on 8.3.x 
    Issue #2804675 by alexpott, hideaway: Tests using TestDatabase class are...

  • catch committed a066b50 on 8.2.x 
    Issue #2804675 by alexpott, hideaway: Tests using TestDatabase class are...
catch’s picture

catch
he/him
Primary language English
commented
Version: 8.2.0-rc2 » 8.2.x-dev
Status: Reviewed & tested by the community » Fixed

I think that's a good, honest, limitation.

Added a change record: https://www.drupal.org/node/2806653

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.