Resolve infinite stampede in mtime protected PHP storage

Nice research, nice work!

This is of course at least major, if not even critical, given what it solves.

Tagging security.

This is probably one of the most security reviewed code in core, so will need a security review unfortunately.

Patches from #7 is a bit messed up, interdiff.txt is the actual patch and the patch is empty. Fixed this.

+++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
@@ -78,43 +78,26 @@ public function save($name, $data) {
+    rename($temporary_path, $full_path);

Using an atomic file operation here is clever. Let's document why we're doing this.

I am a crufty Drupal core developer that has not been involved for five years, and I have not looked at D8 before this week. So, it is entirely possible that I do not know what I'm talking about. However, I have some concerns with this patch.

First, specific comments on the code:

1. +++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
   @@ -78,43 +78,29 @@ public function save($name, $data) {
   +    rename($temporary_path, $full_path);
   

   This operation can fail, and you are not checking the return value.

2. +++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
   @@ -78,43 +78,29 @@ public function save($name, $data) {
   +    touch($directory . '/', $mtime);
   

   Ditto, even if you know the rename succeeded. For example, a network partition might have just separated you from the NFS server.

3. +++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
   @@ -162,6 +148,37 @@ public function delete($name) {
   +        foreach (new \DirectoryIterator($directory) as $fileinfo) {
   

   The directory might cease to exist between file_exists() and the creation of the iterator.

4. +++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
   @@ -162,6 +148,37 @@ public function delete($name) {
   +              @chmod($directory, 0777);
   +              @unlink($fileinfo->getPathName());
   

   Return codes.

5. +++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
   @@ -162,6 +148,37 @@ public function delete($name) {
   +          $this->unlink($name);
   

   Return codes.

Second, and more importantly, even if you address all of those comments, I'm still concerned. It is commonplace now for Drupal to run in multi-web environments using a distributed filesystem, and distributed filesystems are just not that reliable. A "POSIX compatible distributed filesystem" is basically a myth. It is not safe to assume that rename() is atomic or that touch() operations work or, basically, almost anything else "clever" about a filesystem. So, this patch will result in D8 being not that reliable.

Furthermore, even if I'm wrong and there is some way to make this work reliably using the filesystem (I doubt it), I'm *still* concerned, because then you will have forced D8 to be absolutely dependent on using a distributed filesystem that provides strictly POSIX-compatible rename() operations. There are a number of potentially compelling choices for Drupal's shared filesystem that do not meet this criteria (e.g. S3).

The common component in a Drupal stack that is capable of performing atomic operations is the SQL database. So, for operations that need to be locked or atomic, use the database. Or, use a locking service.

Does D8 have an existing service interface for locking across all web nodes?

> because then you will have forced D8 to be absolutely dependent on using a distributed filesystem that provides strictly POSIX-compatible rename() operations

Noone should use the mtime protected storage on a shared filesystem. That's a piss poor idea. #2301163: Create a phpstorage backend that works with a local disk but coordinates across multiple webheads and #2513326: Performance: create a PHP storage backend directly backed by cache both offer better ideas.

> Storing code on the filesystem has the advantage that we can leverage opcode caching.

Still beating my own drum: both approaches linked in #16 keeps this via different means.

> Especially for small sites this is probably the fastest option for retrieving generated code we currently have.

That is certainly true -- querying a distributed data store to coordinate across webheads will certainly be slower than not querying one :) Once coordination is done, opcache hits so it's a wash from there.

On to this patch, this is a very nice trick, making the directory mtime a (possibly) false mtime, very nice, requires PHP 5.3.0 but we are way past that.

Should we find a way to totally remove mtime storage before release if it's not reliable? Is this issue or the linked issues critical?

It feels like we should consider also facilitating the pattern of committing the built PHP to git and blocking config changes in production?

It feels like we should consider also facilitating the pattern of committing the built PHP to git and blocking config changes in production?

Locking down is all good and well, but at some point, you need to do actually do a config deployment, which can absolutely include new modules.

I really don't see how that is supposed to be possible unless you also deploy the whole database from staging which already has the new modules installed. Or have multiple prepared containers ready, which then depend on the list of enabled modules or something like that.

Or have multiple prepared containers ready, which then depend on the list of enabled modules or something like that.

We've got a bit closer to that with the addition of $deployment_identifier in settings.php, although that would mean the new container is used immediately on code deployment and before update.php is run or config is synced, so it'd still be out of sync until both of those are done. You could go further and write the deployment hash to memcache then read it in settings.php but that gets really nasty fast.

#2497243: Replace Symfony container with a Drupal one, stored in cache means the container is no longer written to PHP (and is critical), so the container will be completely out of phpstorage.

However #2429659: Race conditions in the twig template cache - twig templates are still using phpstorage.

It would be possible to use read-only phpstorage for twig templates, but they can differ based on enabled modules (due to node visitors http://twig.sensiolabs.org/doc/advanced.html) - so while that could be pre-generated in a build step on a per-site basis, we can't add template building to d.o packaging etc. as it stands.

Going to bump this to critical on the basis that:

- It fixes #2497243: Replace Symfony container with a Drupal one, stored in cache (even if I think that issue is release blocking for other reasons).

- Also fixes #2429659: Race conditions in the twig template cache even though we worked around it in twig - although the workaround requires an eval() which still concerns me a bit.

- Any other use of phpstorage will have the same race condition/stampede issue until this is in.

Note #2547827: PhpStorage: past, present and future

I done a line-by-line review of this. The use of atomic operations to avoid race conditions is the way to go. Nice work @znerol.

If you what to repeat the test result you need to run the test without a parent site installed and have no settings.php at all. You need to use the sqlite test runner. Something like:
php ./core/scripts/run-tests.sh --sqlite /tmp/coretest.sqlite --dburl sqlite://localhost/sites/default/files/db.sqlite --color --url http://your.url

    if (!$this->addServiceFiles(Settings::get('container_yamls'))) {
      throw new \Exception('The container_yamls setting is missing from settings.php');
    }

Is this code actually valid? Is it really mandatory to have a container_yamls setting in settings.php. This looks wrong to me. Simpletest sets it to an empty array a lot so that this does not thrown an exception.

Afaik #2547447: Stop creating services.yml by default will get rid of that requirement, so strictly speaking I don't think we would really need it.

So the other issue fixed indeed the test failures ... back to RTBC

Really good to see this green. Committed/pushed to 8.0.x, thanks!