Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: content
Choose a base branch
from
Open

Fix WindowsError/OSError in singleton #10037

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion Monika After Story/game/python-packages/singleton.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link

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.

Copy link
Member Author

@dreamscached dreamscached Jul 3, 2023 •
edited
Loading

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.

Copy link

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"?

Copy link
Member

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.

Copy link
Member Author

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.)

Copy link

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.

Copy link
Member

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.

os.unlink(self.lockfile)
try:
os.unlink(self.lockfile)
except OSError:
# Race conditions happen, make sure the file is actually deleted
# Carry on with the startup if it is
if os.path.exists(self.lockfile):
raise

self.fd = os.open(self.lockfile, os.O_CREAT | os.O_EXCL | os.O_RDWR)

except OSError as e:
Expand Down