[5.7] Fix filesystem locking hangs in PackageManifest::build()

[Yogarine]

(this time with less code)

See #25898 PackageManifest::build() hangs on filesystems that don't support exclusive file locks

• Filesystem.php

  1. Created a new Filesystem::replace() method that atomically
     replaces a file if it already exists.

• PackageManifest.php

  1. The Filesystem::get() call in PackageManifest::getManifest() no
     longer has to use an exclusive lock because the packages.php
     manifest file will always be replaced atomically.

  2. Use the new Filesystem::replace() method in
     PackageManifest::write()

[sisve]

So this is put() with some permission checks? Couldn't those permission checks be added to put() instead of introducing a new method?

[Yogarine]

So this is put() with some permission checks? Couldn't those permission checks be added to put() instead of introducing a new method?

No. The replace() function works different from put() and has different requirements and use cases.

It requires the parent directory of the file to be writeable, because it creates a temporary file next to the target file. put() doesn't have this requirement, it only needs the target file to be writeable.

It also makes sure the permissions of the new file are the same as the original file.

[mfn]

Does this filepermsmagic work on windows?

[Yogarine]

Good point. The PHP docs on fileperms() doesn't mention Windows. I'll have to test this on a Windows machine.

If it doesn't, I'll just remove this line. That will make it function a little more different from put() in the corner case where the permissions of the existing file don't match those of the file it is replaced with. But for the places where we would want to use it it's still appropriate. replace() isn't a perfect substitute for put() to begin with.

Perhaps I am just overthinking things and I should remove the chmod() and chgrp() altogether and just make it clear replace() will also replace the ownership and permissions of the file.

[Yogarine]

So I did what I was considering; I removed the chmod() and chgrp(). Also mentioned what happens to file permissions in the PHPDoc. This also solves any potential Windows incompatibilities.

[taylorotwell]

It really takes all of this to simply replace a file's contents in PHP? We should be running all this code every time we need to do this? I feel like we are adding a bunch of code just to solve an edge case that has never been reported for 2 years until now?

[Yogarine]

It really takes all of this to simply replace a file's contents in PHP? We should be running all this code every time we need to do this? I feel like we are adding a bunch of code just to solve an edge case that has never been reported for 2 years until now?

This "edge case" is new since Laravel 5.6.30, commit 683637a5c0d8ee0049e9067785175f9a291e824e to be exact: To fix race conditions when bootstrap/cache/packages.php is written exclusive locking was enabled when reading and writing the packages.php file (exclusive locks hadn't been used in Laravel before that). Even though it may not very common, the impact is pretty big (io deadlocks), and it took us totally by surprise after pushing a Laravel bump from 5.6.25 to 5.6.39 to our production server.

Exclusive locks aren't supported all file systems, notably NFS3 which is still widely used. In hindsight, since exclusive locks are a new external requirement for Laravel, that fix shouldn't even have been released as a patch update.

For more information see issue #25898.

I really don't think it's that much code either. There is some sanity checking going on. I'm already considering removing the chown() and chmod() because they might be overkill. I could also remove the realpath() check above, at the risk of overwriting the symlinks to a file with the replacement file itself, instead of replacing the symlink target.

[mfn]

Shower thought: what if locking here is a config setting, default on, which can be turned off, would that work?

Then it wouldn't require all this code but simply switched the value of the 3rd arg passed to the get/put method (looking at 683637a which you referenced).

Maybe it's a bad either, but it's "simple".

[Yogarine]

Shower thought: what if locking here is a config setting, default on, which can be turned off, would that work?

I thought about that at first, but not sure if it's smart to depend on configuration during the bootstrap process. Of course, you could just default to off, and then carefully check if config is available. Not sure if that would be less new code than what we have in this PR though.
And there is a very legit reason exclusive locks were introduced. Overriding the packages.php manifest file while another process is reading it can cause corrupt manifests to be loaded. Worse even: if two processes try to write the manifest at the same time, you get a corrupt manifest file. So you shouldn't really every want to have to disable this because it can cause nasty issues.

The advantage of the replace() approach over exclusive locks is that it is fully atomic and doesn't interrupt any other requests, so it should perform better, and doesn't require exclusive locks at all.

[gmutinel]

In my Ubuntu system I have a problem now, every time I launch the composer install command it resets the permissions on the bootstrap/cache/package.php file giving continuous permission denied errors.
I manually reverted the changes from this commit (specifically only the ones in the file src/Illuminate/Foundation/PackageManifest.php) and everything is working again. The directory is not owned by the WebServer user (I know, there are reasons though) but we have tons of projects working perfectly this way up to Laravel 5.7.9, looks like a breaking change to me

[Yogarine]

In my Ubuntu system I have a problem now, every time I launch the composer install command it resets the permissions on the bootstrap/cache/package.php file giving continuous permission denied errors.
I manually reverted the changes from this commit (specifically only the ones in the file src/Illuminate/Foundation/PackageManifest.php) and everything is working again. The directory is not owned by the WebServer user (I know, there are reasons though) but we have tons of projects working perfectly this way up to Laravel 5.7.9, looks like a breaking change to me

Ah, that sucks. I imagined this might possibly happen to people that symlinked the bootstrap/cache/package.php file. However, I really do think that bootstrap/cache should always be owned by the www-data user for Laravel to work reliably.

You might be able to fix this with ACLs. Try:
setfacl --logical --mask -m u:"www-data":rwX YOUR_BOOTSTRAP_CACHE_DIR

[laurencei]

I imagined this might possibly happen to people that symlinked the bootstrap/cache/package.php file

You really should have mentioned that in the original PR, so that Taylor could have been aware of all the facts and considered it as part of his decision to merge the PR.

[Yogarine]

You really should have mentioned that in the original PR, so that Taylor could have been aware of all the facts and considered it as part of his decision to merge the PR.

I did.

[laurencei]

It's not obvious re-reading your text of the potential break.

Anyway - regardless - now I'm wondering if the original issue that caused your problems in 5.6.30 should also be reverted.

Is there any other way to solve all these problems?

[Yogarine]

I think the way to cover all bases is to check for proper permissions of package.php's parent dir first, and then decide whether to use the write() or replace() methods.

[Yogarine]

@gmutinel

In my Ubuntu system I have a problem now, every time I launch the composer install command it resets the permissions on the bootstrap/cache/package.php file giving continuous permission denied errors.

The directory is not owned by the WebServer user (I know, there are reasons though)

What bothers me: how does Laravel ever create the packages.php when it doesn't exist yet, if you don't have write access to the cache dir?

[Pillmayr]

One possibility would be to replace the tempnam function inside the \Illuminate\Filesystem\Filesystem::replace function.

file_put_contents(tempnam("/tmp", "123test"), "content");

-rw------- 1 www-data www-data 4 Okt 24 11:11 123testRoXZmt

file_put_contents("/tmp/123test", "content");

-rw-r--r-- 1 www-data www-data 4 Okt 24 11:11 123test

[Yogarine]

@Pillmayr Ah wow that's right.

Originally I actually did an additional chmod() to replicate the target packages.php's files to the new file, but that didn't really make a lot of sense because the permissions are also dependant on ownership, so I removed it. I never realised tempnam() resets the permissions. (Or I forgot about it.)

I think the proper fix is to add a chmod() to reset the permissions after the tempnam().

[Yogarine]

To be more precise, tempnam() actually creates the file.

So, not as pretty, but adding an unlink() to remove the tempfile should also work. (And make sure the current umask is respected.)

[Pillmayr]

Did not read the full doc of tempnam and realized afterwards, that the function already creates the file. So you are completely right: changing permissions after creating temp file would be the best solution.

[Yogarine]

@Pillmayr

Did not read the full doc of tempnam and realized afterwards, that the function already creates the file. So you are completely right: changing permissions after creating temp file would be the best solution.

How about unlink()-ing the file?

[Pillmayr]

Didn't catch your idea first. :D But for me the outcome is the same.
The only question that arises for me: The user never sees the generated file anyway. This is just about preventing a "dead lock", right? Why not just create a temporary filename using uniqid? Without a file operation. (Creating temp file, changing permission or deleting it anyway) In my opinion this would be the fastest and most uncomplicated variant.
What do you think?

[Yogarine]

Using uniqid() probably also works but doesn't check if the file already accidentally exists. (I know, conflicts are extremely rare but theoretically possible.)

I like that tempnam() is specifically made to create a temp file. It's also nice that it already ensures that the file can be created. We just need to remove it afterward.