-
Notifications
You must be signed in to change notification settings - Fork 685
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
Fix WindowsError/OSError in singleton #10037
base: content
Are you sure you want to change the base?
Fix WindowsError/OSError in singleton #10037
Conversation
@@ -36,7 +36,14 @@ def __init__(self, flavor_id=""): | |||
# file already exists, we try to remove (in case previous | |||
# execution was interrupted) | |||
if os.path.exists(self.lockfile): |
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 addition of the try..except makes this check redundant. Remove it and de-indent the try..except block.
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.
No, it is not. We have already discussed this with @Booplicate and came to the conclusion this solves an issue some players may have when due to race condition file no longer exists after os.path.exists
check, which causes an unnecessary error screen.
try/except is used to catch this race condition caused error, so we can re-verify the existence of that file and throw an error if it still there and was not removed by another instance that beat this instance to it.
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 was not removed by another instance that beat this instance to it.
This smells like some sort of global state to me, but I'm admittedly unfamiliar with this codebase. Are you certain this instance should handle the lock file's deletion in that case?
At any rate, you already end up checking twice in the try..except alone. The .unlink()
attempt is the implied check and a second one to handle the race condition happens in the except
block. Is the if
I marked really needed at this point, since all it's doing is "if this exists, check if it exists while I delete it, then check again if exceptions occur"?
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 smells like some sort of global state to me
Which is why it's called singleton pattern, you can't do singleton without a global state.
Are you certain this instance should handle the lock file's deletion in that case?
The comment above the line you selected explains why we're trying to delete it.
Technically, you are correct, the system will check if the file exists when executing the unlink
os call. However the check ensures that the file was present before the call to unlink
.
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.
Well yes, it is a global state, we're talking about a lock file that is used to ensure only one instance of an entire application runs.
I did suggest to leave it just as os.unlink
but was told it shouldn't remove this file for no reason (and that the preliminary check is necessary.)
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.
Technically, you are correct, the system will check if the file exists when executing the unlink os call. However the check ensures that the file was present before the call to unlink.
Sure, however the purpose of try..except is also to cut down on preliminary checks where possible. This seems to be one of the cases where it is possible.
The comment above the line you selected explains why we're trying to delete it.
Makes sense, yes. Alternatively, I wonder if one could query the list of running processes and abort execution in case another MAS is already running, but I'm unfamiliar with Windows' way of doing this.
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.
I don't feel like changing existing code to be more of a cross-platform nightmare than it already is unless there's a strong argument for it.
Changelog: