Fix file locking on Windows #3347
Merged
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
Windows cannot erase directories if there are open file handles. Limited mechanism added for tracking lock handles to allow the .opam directory to be deleted if an error occurs during opam init. Signed-off-by: David Allsopp <david.allsopp@metastack.com>
The underlying function for flock on Windows doesn't support promotion of read locks or demotion of write locks and the locks are also not recursive. Signed-off-by: David Allsopp <david.allsopp@metastack.com>
On Windows, locked files should not be opened twice even within the same process, so utilise the adapted OpamFilename.with_flock functions to prevent the need for a second fd. Signed-off-by: David Allsopp <david.allsopp@metastack.com>
|
Seems fine, thanks! |
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.
Another bit from #3260.
These three commits are worth looking at individually.
Close all lock files on error: this, as you noted in Fully test native Windows in the testsuite #3260, this shouldn't be necessary, but given that any coding error could result in this, I'd prefer to keep it. It could possibly be extended to display warnings ifOpamSystem.release_all_locksactually has to release any locks? It would seem to be much better to have warnings displayed, which need fixing, than to get error messages about being unable to remove a directory (in this case~/.opam), as failing to delete the directory in this instance will probably result in worse error messages for the user on the next command they run.Fix file locking on Windows: this is sound, though could potentially result in the loss of the lock to another opam process. This could be fixed by converting locks to be pairs of read and write locks if you want? A benefit of doing that is the code would cease to be Windows-specific (it would just retain a comment for why it can't be done the more obvious way).Fix reading/writing of OpamRepositoryState cache: is a simple refactoring to prevent opening a file twice, since this doesn't work on Windows if it's locked for writing (even by the same process).