Add Support for locking the registry #1828
Conversation
|
-launch gui |
|
What happens when you run CLI with no params without having the GUI open? |
politas
added
GUI
|
yes, but showing the error to the user twice (once in cli, once in gui) is not really needed. |
|
There is a weird case where we are failing to create a lock file issue. When you start a second GUI instance selecting a different KSP install and then switch to the same install as the first instance. The second instance is killed, and all is well unless you then open a different KSP install with the first instance. Under that situation, it doesn't open a new lock file, and if you keep switching KSP installs it keeps not creating lock files. I can't find any other cases where lock files are not handled correctly, and this is going to be a pretty rare situation. But if anyone would like to try as many situations as they can imagine, any further issues might help to pin down the flaw. |
|
Further investigation shows the second KSP instance is not the culprit. Returning to a KSP Install in which you have already had a lock file created and removed will not create a new lock file. |
|
Ok, the only remaining issue I can find is the duplicated error messaging, and I'm happy to live with that. |
|
needs more testing |
|
Rebased after #1841 |
| //Don't try to Dispose a null CurrentInstance. | ||
| if (CurrentInstance != null) | ||
| { | ||
| // Dispose of the old registry manager, to release the registry. |
There was a problem hiding this comment.
Possible null dereference on RegistryManager.
|
Per https://msdn.microsoft.com/en-us/library/ms229043.aspx and https://msdn.microsoft.com/en-us/library/ms229045(v=vs.110).aspx
Some of the variables and parameters are in snake_case when you want them in camelCase. I also added a few other style notes - not trying to be critical of your work at all in case it came off that way. |
| // Release the registry lock file if possible. | ||
| if (manager.CurrentInstance != null) | ||
| { | ||
| manager.CurrentInstance.RegistryManager.Dispose(); |
There was a problem hiding this comment.
If manager.CurrentInstance.RegistryManager is null, this will throw a NullReferenceException. Take a look here for an elegant solution to these kinds of issues.
|
Oh of course. Mono 3.2.8 doesn't support Null Conditional Operators. Rolled that back to the ugly method, now improved to catch all possible nulls |
|
|
||
| ///public void Dispose() | ||
| ///{ | ||
| /// this.RegistryManager.Dispose(); |
There was a problem hiding this comment.
If this remains commented out, an additional comment explaining why it was commented out or what it did would be helpful.
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.
Need to drop the KSP registry information from the singleton[] array on RegistryManager.Dispose() or we will not create a new lock file if we return to a previously locked KSP Install
|
Ah yes, that little bug! Whacked again. |
Closes #706
Working from previous PRs #1357, #1265