Follow-up for #5205: fix high concurrency race condition

[wimleers]

Composer would fail with an

PHP temp directory (/tmp) does not exist or is not writable to Composer. Set sys_temp_dir in your php.ini

error when used in parallel with a sufficiently high rate of operations. Because it is checking if a file with md5(microtime()) can be created, which is not sufficiently unique when used in parallel:

            // Check system temp folder for usability as it can cause weird runtime issues otherwise
            Silencer::call(static function () use ($io): void {
                $tempfile = sys_get_temp_dir() . '/temp-' . md5(microtime());
                if (!(file_put_contents($tempfile, __FILE__) && (file_get_contents($tempfile) === __FILE__) && unlink($tempfile) && !file_exists($tempfile))) {
                    $io->writeError(sprintf('<error>PHP temp directory (%s) does not exist or is not writable to Composer. Set sys_temp_dir in your php.ini</error>', sys_get_temp_dir()));
                }
            });

Since each Composer instance runs in its own process, this can easily be mitigated by not just partitioning based on time of use (microtime()), but also based on process ID (getmypid()). 🥳

Original investigation: https://www.drupal.org/project/automatic_updates/issues/3338789#comment-14961390

History

#5205 introduced this in 2016 at 28e9193 and it was refined a few days later in 43eb471 — since then it has been untouched.

Who is affected?

It would seem that Drupal's Automatic Updates & Project Browser functionality (see https://dri.es/drupal-project-browser-empowers-ambitious-site-builders) is the first to run into this. Or to be more precise: its test coverage is the first to run into this, because Drupal core's test suite is so enormous that it has to be massively parallelized in order for it to run in any reasonable amount of time (currently ~1 hour with concurrency = 32).

Work-around until this lands

We added the following to our \Drupal\package_manager\ProcessFactory::create() which is a Symfony Process factory which we use to trigger composer from within our test suite:

      // Work around Composer not being designed to be run massively in parallel
      // which it may in the context of Package Manager, at least for tests. It
      // is trivial to work around though: create a unique temporary directory
      // per process.
      // @see https://www.drupal.org/i/3338789#comment-14961390
      // @see https://github.com/composer/composer/commit/28e9193e9ebde743c19f334a7294830fc6429d06
      // @see https://github.com/composer/composer/commit/43eb471ec293822d377b618a4a14d8d3651f5d13
      static $race_condition_proof_tmpdir;
      if (!isset($race_condition_proof_tmpdir)) {
        $race_condition_proof_tmpdir = sys_get_temp_dir() . '/' . getmypid();
        // The same PHP process may run multiple tests: create the directory
        // only once.
        if (!is_dir($race_condition_proof_tmpdir)) {
          mkdir($race_condition_proof_tmpdir);
        }
      }

… and immediately our test suite began to pass consistently!

We can keep doing this in our end, but I see no reason why Composer would not want to be more robust 😊

[wimleers]

The failures here are for PHPStan compliance, but the single line that this PR changed is not being complained about … 😅

Note: this is somewhat related to #9580, since this also relates to improving atomicity.

[stof]

phpstan currently fails in the master branch already.

[vtsykun]

Can we use random_bytes, just Composer already support php 7.2?

[stof]

random_bytes would not guarantee that we don't have race conditions. It would just make them more random.

[wimleers]

Indeed.

It'd reduce the collision rate, but not prevent it.

Namespacing by process ID prevents it.

[Seldaek]

Thanks, merged as 5d2d513 in 2.5 branch