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

Fix race conditions #1498

Merged
merged 1 commit into from
Apr 8, 2019
Merged

Fix race conditions #1498

merged 1 commit into from
Apr 8, 2019

Conversation

pb-cdunn
Copy link
Contributor

@pb-cdunn pb-cdunn commented Apr 6, 2019

This is a "Heisenbug", a race condition. You might not be able to reproduce it, but I'll describe it anyway.

We're running this as an automated test, as a user we cannot control, so use export PLOTLY_DIR=whatever

re: #1076, addressed in #1195

This is still a problem with plotly-3.7.1. In plotly/files.py:

 27 def _permissions():
 28     try:
 29         if not os.path.exists(PLOTLY_DIR):
 30             os.mkdir(PLOTLY_DIR)
 31         with open(TEST_FILE, 'w') as f:
 32             f.write('testing\n')
 33         os.remove(TEST_FILE)
 34         return True
 35     except:
 36         return False

Lines 30 and 33 can fail, wrongly, if two processes run this at the same time. (f.write() is fine here, IMO.)

Note that I want to make warnings into errors in my tests. (pytest -W error), so this is a big problem for me even if you simply trap and warn().

I'm not sure if there is still a warning in 3.7.1, but like I said, it's a Heisenbug. I can only spend so much time on this. For me, it was happening via pytest-xdist, in a case where I lacked permissions to create ~/.plotly. PLOTLY_DIR lets me reduce the likelihood of error, but the race conditions remain.

@pb-cdunn
Copy link
Contributor Author

pb-cdunn commented Apr 6, 2019

Also, bare except: would trap KeyboardInterrupt, which is usually not intended.

@jonmmease jonmmease added this to the v3.8.0 milestone Apr 8, 2019
@jonmmease
Copy link
Contributor

Thanks for digging into this @pb-cdunn, I haven't run into this before but your reasoning and these changes both make sense so I'm happy to merge this.

Regarding warnings, my intention in #1195 was to only raise a warning if a user explicitly asks to perform an operation that needs write access to the PLOTLY_DIR directory, and this write cannot be performed.

@jonmmease jonmmease merged commit 133df4d into plotly:master Apr 8, 2019
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.

2 participants