-
-
Notifications
You must be signed in to change notification settings - Fork 345
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
LockFiles with adjustments #1357
Conversation
I opened the gui. I then wanted to install OPM with the cli. Got this error:
|
Hooray! We should probably make that more user-friendly, shouldn't we? But hooray for the correct kraken being thrown. |
Yes, the user friendly message should be shown. I ran the Tests. Most of them throw a "The process cannot access the file "registry.json.locked" because it is being used by another process." now. |
lockfile_stream = new FileStream(lockfile_path, FileMode.CreateNew, FileAccess.Write, FileShare.None, 512, FileOptions.DeleteOnClose); | ||
|
||
// Write the current process ID to the file. | ||
StreamWriter writer = new StreamWriter(lockfile_stream); |
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.
The StreamWriter should be closed and disposed.
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.
Oh, good catch! We need to keep the FileStream
open, but not the StreamWriter
.
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.
But we can't close the StreamWriter, as that would close the underlying file.
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.
And we can't dispose it either, as that also releases the lock. Adjusting code now.
Updates applied include:
|
Opening the gui, then closing it throws this:
|
@Postremus : Resource disposal adjusted! Travis seems to like this better, too. :) |
The Tests still throw an "The process cannot access the file "registry.json.locked" because it is being used by another process." at me. |
@Postremus : I've found a very obvious suspect for the locking issues when running tests! I hope badfc43 brings great joy! |
@Postremus : And another patch, but I suspect you'll see at least one more bug, but we're almost there. :) |
// This is not entirely necessary, but we can show a nicer error message this way. | ||
try | ||
{ | ||
#pragma warning disable 219 |
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 is this needed?
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 may be one for @mgsdk (who authored the change). I suspect calling RegistryManager.Instance(CurrentInstance).registry
without an assignment generates a warning and/or requires an assignment, but I honestly haven't tested that.
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.
It is for assigning a variable but never using it, CS0219. Since I know it wouldn't be used, I thought I would reduce the warnings in the output.
Note: This makes the `lockfile_path` public so our tests can get to it, but it's a readonly var so nobody can change it.
* Saves a variable and many lines of code. * Improves readability, a bit...
Also, I totally love finally blocks.
Disposing of the writer also disposes of the stream, but the other way around (which we had) causes great sadness, or at least an unhandled exception.
This should make windows users happy, as otherwise the files can't be deleted at all!
This means our tests (and possibly other code) can get it to release locks properly.
|
This still requires a rebase and, I assume, a solution to what @Postremus posted above. |
Closing this in favour of #1828, which continues the work |
This is #1265, but with the addition of tests and extra garbage collection. Specifically we need reviewed: