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.

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

set_maintenance_mode should use atomic write. #162

Closed
reitermarkus opened this issue Dec 10, 2023 · 8 comments
Closed

set_maintenance_mode should use atomic write. #162

reitermarkus opened this issue Dec 10, 2023 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@reitermarkus
Copy link

reitermarkus commented Dec 10, 2023

Python version

any

Django version

any

Package version

any

Current behavior (bug description)

set_maintenance_mode uses normal write, i.e open + write, which may result in a race condition as seen in inventree/InvenTree#6066. An empty maintenance_mode.txt then leads to a state value is not 0|1 error.

Expected behavior

set_maintenance_mode uses atomic write, i.e. open + write to temporary file, then mv temporary file to actual file. I'm sure there is a Python package that provides such a function.

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@reitermarkus reitermarkus added the bug Something isn't working label Dec 10, 2023
@fabiocaccamo
Copy link
Owner

@reitermarkus thank you for reporting this issue.

@fabiocaccamo
Copy link
Owner

Depends on fabiocaccamo/python-fsutil#91

@fabiocaccamo
Copy link
Owner

@reitermarkus fixed in 0.21.0 version.

@reitermarkus
Copy link
Author

Thanks! Although I would remove

fabiocaccamo/python-fsutil@6c8a58b#diff-d82a200cf6742866f0d549f5bd4a22416ef6cdbcc1cde15c6b32c64f2663fe70R1330

since this can again have the same race condition and pollute user directories with temporary files.

@fabiocaccamo
Copy link
Owner

@reitermarkus for the temp file directory I used the same file directory to ensure that the temporary file is created on the same filesystem of the final file and guarantee atomicity of the replace operation, otherwise there is a potential risk of data corruption or loss.

What do you think is best to do here?

@reitermarkus
Copy link
Author

Given that the state can only be 0/1, maybe change to simply checking the existence instead of the contents of the file.

@fabiocaccamo
Copy link
Owner

The local file backend uses the same logic as the other backends.

@fabiocaccamo
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

2 participants