Fix race condition when deleting pidfile#3657
Conversation
When the pidfile is removed between File.exist? and File.unlink.
|
This was an issue when running benchmarks with puma, subscribed to this PR |
| path = @options[:pidfile] | ||
| File.unlink(path) if path && File.exist?(path) | ||
| begin | ||
| File.unlink(path) if path |
There was a problem hiding this comment.
| File.unlink(path) if path | |
| File.unlink(path) if path && File.exist?(path) |
Only do the rescue if race conditions occur, this would make it rescue even without race conditions (which I assume would have a performance impact)
There was a problem hiding this comment.
It should commonly be the case that the pidfile exists; in the expected case that path is non-nil and the file exists then the single syscall to unlink(2) should be cheaper than the two syscalls, and in the unexpected case where path is non-nil and the file doesn't exist, the syscall to stat(2) should not be cheaper than the syscall to unlink(2). Any difference should come from raising the exception, but as stated this should already be the exceptional case. We should also not expect the call to delete_pidfile to be called in a performance-critical path, but if this is a concern, this should be verified.
There was a problem hiding this comment.
Agree if this is the edge case that the file does not exist.
| path = @options[:pidfile] | ||
| File.unlink(path) if path && File.exist?(path) | ||
| begin | ||
| File.unlink(path) if path |
There was a problem hiding this comment.
We are still making a change to how this function works regardless of whether there are race conditions or not. So the question is if there are other errors that arise when not checking File.exist?(...) apart from the ENOENT.
ENOENT: A component in pathname does not exist or is a dangling
symbolic link, or pathname is empty.
Given the definition of the error after the syscall, I am assuming the behaviour is the same as the File.exist? but just without race conditions
and
Not sure what is causing the above, but the changes in this PR are reasonable. |
Description
In a concurrent environment, a race condition can occur if the file is deleted between the call to
File.exist?and the call toFile.unlink. In this case,File.unlinkwill raise anErrno::ENOENTexception because the file no longer exists. This change removes the explicitFile.exist?check and instead safely rescuesErrno::ENOENT.Your checklist for this pull request
[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.