Improve FileMutex to use amphp/file if available
enumag opened this issue · 12 comments
I noticed that the FileMutex is using the \unlink() function. But isn't this blocking? Shouldn't \Amp\File\unlink() be used instead?
https://github.com/amphp/sync/blob/master/src/FileMutex.php#L77
Same applies to fopen, yes.
Is it okay to add require amphp/file to composer.json?
I don't want it to use async API magically when available. Seems weird.
Imo we should either require amphp/file or throw exception if it isn't installed.
Throwing would be a BC break. amphp/file depends on amphp/parallel, I'd rather avoid that dependency in amphp/sync, as amphp/cache depends on amphp/sync and amphp/dns depends on amphp/cache, where we removed amphp/file as a dependency some time ago on purpose.
Then maybe we should add a separate class that would use the amphp/file or throw?
AsyncFileMutex or something?
and rename FileMutex to BlockingFileMutex with alias for BC I'd say...
We can add AsyncFileMutex to amphp/file, not sure whether we need the rename.
If it's in amphp/file then it becomes kinda hidden and ppl won't notice it. Renaming is necessary in my opinion to make it obvious that this class shouldn't actually be used.
amphp/file is the right place for it, because it already depends on amphp/sync through amphp/parallel. If we release a new major version of amphp/file and have just a check here, things might break or get really hacky if we need to adapt.
Discoverability is can probably be solved via documentation.
Alright, I'll try to implement it and open a PR at amphp/file. Thanks!