feat: flock db dir #500
Merged
feat: flock db dir #500
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
pepyakin
changed the base branch from
pep-feat-configurable-rollback-tp-size
to
pep-chore-fix-macos-build
Merged
pepyakin
commented
Oct 21, 2024
| @@ -29,6 +29,7 @@ lru = "0.12.3" | |||
| libc = "0.2.155" | |||
| criterion = { version = "0.3", optional = true } | |||
| thread_local = "1.1.8" | |||
| fs2 = "0.4.3" | |||
There was a problem hiding this comment.
This is not my favorite, as this doesn't give us much. We don't care about supporting much besides modern Linux and macOS, we don't explicitly target Windows and can just directly use flock. But I thought for now this is OK and we should reconsider that in #501.
| impl Drop for Flock { | ||
| fn drop(&mut self) { | ||
| if let Err(e) = self.lock_fd.unlock() { | ||
| eprintln!("Failed to unlock directory lock: {e}"); |
There was a problem hiding this comment.
I know this is the first appearance of eprintln! in this codebase. Should we finally add tracing or log? Or should we just swallow this error? Or leave it as is for now?
There was a problem hiding this comment.
This should really only happen very rarely. An eprintln! seems fine, and it'd at least give an indication of what went wrong that can come back up to us.
rphmeier
approved these changes
Oct 21, 2024
Closes #483 This commit aim is to return an error message if another process tries to use the same db directory, which is unsupported according to our assumptions. Note that this uses only advisory locking. It's still possible to mess with the directory and containing files. It's the responsibility of the user to prevent that and failure to do so can and will lead to unexpected behavior.
pepyakin
added a commit
that referenced
this pull request
Nov 5, 2024
fs2 was introduced in PR #500, but we actually don't really need that much from it. This commit removes the extra dependency in favor of doing the same directly. I think this is pretty-much equivalent to inlining the code.
Merged
pepyakin
added a commit
that referenced
this pull request
Nov 5, 2024
fs2 was introduced in PR #500, but we actually don't really need that much from it. This commit removes the extra dependency in favor of doing the same directly. I think this is pretty-much equivalent to inlining the code.
pepyakin
added a commit
that referenced
this pull request
Nov 5, 2024
fs2 was introduced in PR #500, but we actually don't really need that much from it. This commit removes the extra dependency in favor of doing the same directly. I think this is pretty-much equivalent to inlining the code.
pepyakin
added a commit
that referenced
this pull request
Nov 5, 2024
fs2 was introduced in PR #500, but we actually don't really need that much from it. This commit removes the extra dependency in favor of doing the same directly. I think this is pretty-much equivalent to inlining the code.
rphmeier
pushed a commit
that referenced
this pull request
Nov 5, 2024
fs2 was introduced in PR #500, but we actually don't really need that much from it. This commit removes the extra dependency in favor of doing the same directly. I think this is pretty-much equivalent to inlining the code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #483
This commit aim is to return an error message if another process tries
to use the same db directory, which is unsupported according to our
assumptions.
Note that this uses only advisory locking. It's still possible to mess
with the directory and containing files. It's the responsibility of the
user to prevent that and failure to do so can and will lead to
unexpected behavior.
Implementation Notes
A couple of words on my research of the problem and the taken approach.
There are mandatory and advisory locking. Mandatory locking is a feature of OS that reserves an exclusive right for a specific program to access a particular file. On Linux, mandatory locking is turned off by default and to be avoided (see this). macOS doesn't support mandatory locking.
The typical way to solve this issue is to use advisory locking. That is, depend on the cooperation between the processes that use the files and directories in question. Obviously, this depends on a good will of those processes and not a ironclad solution.
On Linux, it's possible to lock a directory.
flockwith the fd of the directory open with O_PATH can be used. macOS doesn't support opening fds with O_PATH, so this option is not available. To make the solution cross-platform, I stuck to creating a lock file named.lockin the DB directory and obtaining an exclusive lock on that.Now, the original TODO and issue (#483) mentions the TOCTOU issues. Namely, the fact that the checking if the db directory exists, opening the db dir, initializing the db, opening the db files are all non-atomic and it's possible that the state of the filesystem has changed between the checks and file opening or creation, which can lead to all sorts of issues.
Proper implementation should probably employ
openatfamily of syscalls to avoid some of those, at least on Linux. This all is out-of-scope for this PR and better be addressed on the next big refactoring in one fell swoop, e.g. with #501.