-
-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add variant of mutex based on flock #80
base: 3.x
Are you sure you want to change the base?
Conversation
src/FileMutex.php
Outdated
@@ -20,39 +19,16 @@ public function __construct(private readonly string $fileName) | |||
|
|||
public function acquire(): Lock | |||
{ | |||
// Try to create the lock file. If the file already exists, someone else | |||
// has the lock, so set an asynchronous timer and try again. | |||
$f = \fopen($this->fileName, 'c'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not touch
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a one-time cost, not really worth adding a touch here imo
src/FileMutex.php
Outdated
$lock = new Lock($this->release(...)); | ||
|
||
$file->close(); | ||
$lock = new Lock(fn () => \flock($f, LOCK_UN)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will block for a short duration, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered adding a variant that used parallel but decided that the overhead to communicate to/from the IPC worker will be way worse than that of a single syscall that returns immediately.
The disadvantage is that these files will stay in the filesystem. We should likely ship it as a new implementation rather than replacing the existing one? |
I believe there is no reason to add yet another implementation instead of fixing the existing broken one. |
The fact that lockfiles aren't deleted is not a downside, it's precisely the reason why the new implementation is better than the old one. |
That totally depends on the usage of the library. If you're using lots of dynamic files for locks, they'll quickly pile up now. |
Split the new mutex into a separate class. |
Fixes #79