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

test_config: use native tmp_path pytest fixture #328

Merged
merged 1 commit into from
May 26, 2022

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented May 24, 2022

Fixes #42 (see https://github.com/ome/omero-py/runs/6574741655?check_suite_focus=true for the latest occurence of this bug)

The current test_config implementation makes use of the temporary file utility the omero.util.temp_files module. This implementation is not thread safe when running the pytest in a distributed manner as the username ("runner") used for the based directory is the same for all workers.
This creates a potential race condition when from omero.util.temp_files import create_path is invoked concurrently as multiple workers might attempt to use os.makedirs to create the same omero_runner directory.

This PR updates the test implementation to use pytest native tmp_path fixture. Each temporary directory should be unique within the scope of the test so there should be no conflict between workers.

The current implementation uses the omero.util.temp_files module which
is not thread safe when running the pytest in a distributed manner since
the username ("runner") is the same for all groups
@sbesson sbesson requested a review from joshmoore May 24, 2022 15:07
Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting & good find. I do wonder about the possible issues that create_path's non-thread-safety might cause.

This has been in for some time and things are green, 👍

@joshmoore joshmoore merged commit 9418915 into ome:master May 26, 2022
@sbesson sbesson deleted the tmp_path branch May 26, 2022 12:10
@sbesson
Copy link
Member Author

sbesson commented May 26, 2022

I do wonder about the possible issues that create_path's non-thread-safety might cause.

Hopefully the impact should be limited as the issue is related to the creation of the parent directory <OMERO_TEMPIR>/omero_<user>. Since this parent directory is not cleaned up by the library, I think we are dealing with a one-off race condition per user. If this was causing problems with the library usage (rather than the tests), a possible workaround would be to pass exist_ok=True to os.makedirs in TempFileManager.__init__

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Concurrency issue(s) with tox?
2 participants