do not ignore flock() return values for PHP 8.5 compat #60131

xabbuh · 2025-04-03T08:08:07Z

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix #60128
License	MIT

nicolas-grekas · 2025-04-03T08:14:48Z

Rant mode: this is not an improvement, not thankful for being forced to update just fine code...

About the change: what about doing this style instead?

flock(...) || throw new \RuntimeException(...)

xabbuh · 2025-04-03T08:16:00Z

I am not feeling confident with throwing as that means we could throw where we didn't in the past.

stof · 2025-04-03T08:34:31Z

The official documentation of flock suffers from the same issue in the first example of usage: https://www.php.net/manual/en/function.flock.php#refsect1-function.flock-examples

stof · 2025-04-03T08:50:59Z

Why using a (void) cast once and using unused variables elsewhere ?

joelwurtz · 2025-04-03T08:56:25Z

I do believe the right fix would be to revert having this attribute on php for the flock function, IMO this will bring more harm than good.

alexandre-daubois · 2025-04-03T08:59:30Z

In the issue, I suggested to maybe use $_ to show that we explicitly and voluntarily ignore the return value. Having a var name like $lockReleased feels weird because it is not used elsewhere, like we forgot to use it once we have it. I have the feeling that $_ would help with this problem. This notation is widespread in other languages when ignoring a variable

Seldaek · 2025-04-03T09:03:03Z

I had a chat with @TimWolla because it seemed weird to me that LOCK_UN requires a return value check as well.. Conclusion from the discussion is:

flock(..., LOCK_UN) technically still could fail so checking is a good idea still (and anyway the #[\NoDiscard] is added on the function level, regardless of the lock operation).
BUT.. fclose() releases all locks acquired on that handle, and from what I see most usages in this PR are of the flock(..., LOCK_UN); fclose(...) kind, so these flock calls could simply be deleted. (the docs also point this out btw: The lock is released also by fclose(), or when stream is garbage collected.)

nicolas-grekas

There are fundamental design issues with this RFC.

Adding a warning while most of us turn warnings into exceptions - thus ignoring the fact this will create a hard break for many
Adding a workaround that's impossible to use on existing code bases, because new syntax.

I'm really not happy with this. I hope something can be changed in PHP 8.5 before it's released. /cc @TimWolla

src/Symfony/Component/Cache/LockRegistry.php

src/Symfony/Component/Process/Tests/ProcessTest.php

src/Symfony/Component/Cache/LockRegistry.php

nicolas-grekas

Let's wait for php/php-src#18235 to be resolved before merging? I feel like this might be discussed quickly :)

Seldaek · 2025-04-03T09:42:23Z

src/Symfony/Component/Cache/LockRegistry.php

@@ -123,9 +122,9 @@ public static function compute(callable $callback, ItemInterface $item, bool &$s
                }
                // if we failed the race, retry locking in blocking mode to wait for the winner
                $logger?->info('Item "{key}" is locked, waiting for it to be released', ['key' => $item->getKey()]);
-                flock($lock, \LOCK_SH);
+                (bool) flock($lock, \LOCK_SH);


I'd argue this here is technically a bug, if flock fails we probably want to throw or retry instead of just assuming the race-winner is done writing?

if the lock fails, we'll just recompute the value so that's fine ignoring to me

Seldaek · 2025-04-03T09:50:07Z

src/Symfony/Component/HttpKernel/Profiler/FileProfilerStorage.php

@@ -307,9 +307,8 @@ private function doRead($token, ?Profile $profile = null): ?Profile
        }

        $h = fopen($file, 'r');
-        flock($h, \LOCK_SH);
+        (bool) flock($h, \LOCK_SH);


This here too I'd argue should probably be checked?

profiler storage, nothing critical either :)

Sure not critical but why not just throw then in the unlikely event that this fails? If it avoids some corruption and weird outcome it might be worth it but I am not going to argue for this for hours, because indeed it's more of a theoretical problem here.

alexandre-daubois

Feels brittle to rely on the implicit unlock made by fclose(). A contributor unaware of this subtlety could move the fclose() call while refactoring, without thinking about the hidden side effect.

nicolas-grekas

We might want to remove the casts if flock stops yelling after php/php-src#18235 is resolved.
The rest looks fine to me now.

Seldaek · 2025-04-03T10:56:16Z

src/Symfony/Component/Cache/LockRegistry.php

@@ -123,9 +122,8 @@ public static function compute(callable $callback, ItemInterface $item, bool &$s
                }
                // if we failed the race, retry locking in blocking mode to wait for the winner
                $logger?->info('Item "{key}" is locked, waiting for it to be released', ['key' => $item->getKey()]);
-                flock($lock, \LOCK_SH);
+                $locked = flock($lock, \LOCK_SH);


I think this might be a safer fix to check the value, to force it to retry from scratch. This is extremely unlikely anyway I guess, so rather play it safe than risking an inconsistent state below in the try/catch no?

Suggested change

$locked = flock($lock, \LOCK_SH);

if (!flock($lock, \LOCK_SH)) {

continue;

}

nicolas-grekas · 2025-04-04T08:19:18Z

It's been reverted on php-src \o/
Thanks to everyone involved.

carsonbot added Status: Needs Review Bug labels Apr 3, 2025

carsonbot added this to the 6.4 milestone Apr 3, 2025

xabbuh force-pushed the issue-60128 branch from fb09474 to 4055ce6 Compare April 3, 2025 08:10

xabbuh requested a review from jderusse as a code owner April 3, 2025 08:10

xabbuh force-pushed the issue-60128 branch 2 times, most recently from 545d4f8 to 68923bb Compare April 3, 2025 08:28

xabbuh force-pushed the issue-60128 branch 3 times, most recently from 5e9919c to 883a5f4 Compare April 3, 2025 09:20