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

[Yogarine]

• Laravel Version: 5.6.30 - 5.6.38
• PHP Version: 7.1.22
• Database Driver & Version: pgsql, PostreSQL 9.6.6

Description:

The change introduced in commit 683637a5c0d8ee0049e9067785175f9a291e824e causes long hangs and a Warning: file_put_contents(): Exclusive locks are not supported for this stream on our production setup because we use NFS to mount the same codebase on multiple servers.
Our NFS setup doesn't support file locking.

[rant]
File locking is an external feature that isn't guaranteed to be available. Thus forcibly enabling file locking is not something that should be done in a patch release, or at all if you want to write portable code.
[/rant]

My suggestion to fix this is to remove the file locking and use a lockfile in bootstrap/cache instead.

Steps To Reproduce:

1.) Set up an NFS environment to host your code on.
2.) mount the NFS on your host that you serve requests on
3.) Remove the bootstrap cache files so Laravel tries to build them

[mfn]

Steps To Reproduce:

🤔

My suggestion to fix this is to remove the file locking and use a lockfile in bootstrap/cache instead.

I believe a PR to discuss would be welcome.

Btw, did you find a work around? The PackageManifest is registered in \Illuminate\Foundation\Application::registerBaseBindings so for now you might just be able to override it with your own version without locking.

[Yogarine]

I believe a PR to discuss would be welcome.

Btw, did you find a work around? The PackageManifest is registered in \Illuminate\Foundation\Application::registerBaseBindings so for now you might just be able to override it with your own version without locking.

Our current workaround is to just pin laravel/framework on version 5.6.29.

Filesystem::put() with $lock = true is currently used in only three places. The quick fix would be to not have those depend on the $lock argument. I'm also currently trying to figure out if there is a way to detect, from PHP, if exclusive locks are available on a given filesystem.

[mfn]

Are file operations (copy/mv) atomic? Probably not...

But maybe instead of locking/overwriting the existing file, a solution could be to generate a new one in a temporary file and move it over once it's done written.

But if that operation isn't atomic (which I guess it isn't) then this is a dead end anyway.

[Yogarine]

But maybe instead of locking/overwriting the existing file, a solution could be to generate a new one in a temporary file and move it over once it's done written.

Creating a temporary file in Filesystem::put() is not really advisable because you would have to make assumptions about where you could save the temp file and it would cause unexpected side effects when the user just expects a file lock.

Filesystem works fine as an abstraction of the file system, the problem is whatever uses Filesystem shouldn't assume it can use exclusive locks.

Also Filesystem should check if it can lock to prevent a deadlock, and just throw a warning instead. I think using flock() with the LOCK_NB flag can help there.

[Yogarine]

So, question: my fix for this was merged, but do you still accept fixes for the 5.6 branch? In that case I'll make a PR for 5.6 as well.

[you-loan]

+1 I faced with the same problem using NFS

[Yogarine]

@laurencei

You suggested in #26231 that this should be targeted at 5.8. However, considering this was the fix for the permission issue (#26230), do I really need to target 5.8?

The permission issue isn't inherent to the original fix for this issue, but rather caused due to misunderstanding of the tempnam() function. I am kinda annoyed that a revert was chosen over a one-line fix.

I feel that at one side this issue isn't being taken seriously, and on the other side my fix isn't going to be taken seriously either because of that dump oversight. @taylorotwell was already really reluctant to merge my PR. If I just create a new PR identical to #26010 + #26232 it's probably just going to be shot down unless I we clear this out first.

If this means we (@myparcelnl) won't be able to upgrade our Laravel installation until 5.8 comes out, well that seriously sucks.

[laurencei]

@Yogarine - I did the revert PR before your proposed solution. It was up to Taylor which way to go.

If you feel you have a PR that solves the issues, with no breaking changes, then you can create a PR for 5.7 for Taylor to consider. I only proposed 5.8 because of the breaking changes (but you might have a way around that).

I dont have a say on anything relating to this - its 100% up to Taylor and what he considers.

[laurencei]

p.s. couldnt you extend/inject the Filesystem with a custom version - allowing you to upgrade now with your fix?

[Yogarine]

p.s. couldnt you extend the Filesystem with a custom version - allowing you to upgrade now with your fix?

I guess I could do that by overriding Application::registerBaseBindings(). But, knowing myself, that also means this bug is never going to get fixed (by me, at least) because I'll just forget about it.

[mfn]

p.s. couldnt you extend/inject the Filesystem with a custom version - allowing you to upgrade now with your fix?

Or make a package?

Really, I was unsure from the beginning and didn't have a good feeling and too did suggest a local workaround.

If that is possible, you can still release it as a package for others to benefit.

I'm not saying this an issue to be ignored but the give all the merge/revert/discussion/back/forth (also in #26010 ) doesn't give me the impression it's "ready". Too many unknowns of scenarios of how people run Laravel.

[Yogarine]

Well, like I said, the permissions issue isn't an inherent result of the way I tried to fix this. I still stand by my original approach.

The problem was caused by a side effect of tempnam() that I missed. It takes just one line to fix that.

I feel kinda dumb for not having seen that, and I feel even worse that that mistake inconvenienced other Laravel users AND I missed my “shot” at getting this issue fixed upstream.

Anyway, I’ll prepare a new PR with the definitive fix today. I hope Taylor will understand the situation and merge it into 5.7.