[5.8] Write manifest with exclusive lock

[rikwasmus]

On very busy sites, multiple processes could attempt to write to the file, causing race conditions in writing to it with invalid syntax. Lock the file exclusively to avoid parse errors.

[GrahamCampbell]

Please could you update the tests too.

[taylorotwell]

About 99% sure this was already merged and then reverted once. Did you search the PR history.

[rikwasmus]

I can only find #10663, seems unrelated. Would be curious as to what the failure scenario would be while adding the LOCK_EX, seems widely supported enough.

flock() allows you to perform a simple reader/writer model which can be used on virtually every platform (including most Unix derivatives and even Windows).

[rikwasmus]

Last change around the line was 8c67e93f which isn't related either.

[derekmd]

The reverted exclusive-lock attempt was in PackageManifest, not ProviderRepository being changed here.

• PackageManifest::build() hangs on filesystems that don't support exclusive file locks #25898
• PRs [5.7] Fix filesystem locking hangs in PackageManifest::build() #26010 + [5.7] Fix filesystem locking hangs in PackageManifest::build() (#25898) #26254
• Revert [5.7] Revert breaking filesystem changes #26231

[taylorotwell]

I don't think I have any plans to change this on a point release.

[rikwasmus]

Hi,

• I never asked this to be in a point release, I just followed this guide: https://laravel.com/docs/5.8/contributions#which-branch, which told me to MR against 5.8
• is this only closed as a point release request, or just denied?
• this seems more denied on the fact that there is another bug you hit in Illuminate\Filesystem\Filesystem::put() in that case. As it should be easily resolvable with an atomic rename for filesystems which encounter this, I could write a fix for that as well. I won't bother if you currently already know it will be denied though.
• the original issue remains an issue in the current state for high traffic sites
• of note may be that it makes php artisan cache:clear unusable, and incorrect cache needed to be manually deleted, as apparently the file in the cache is amazingly used to clear the cache (but that might be some developers quirk on our side...)

[rikwasmus]

No answer, I'll withdraw my offer of fixing the filesystem bug, and we'll leave it for the next guy to encounter either the broken manifest file due to concurrent reads, or other issues with the exclusive lock writing.

If someone encounter this later: probably just do an atomic rename() in Illuminate\Filesystem\Filesystem::put() instead of an LOCK_EX if you got the energy.