Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix race conditions #1498

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

Merged
jonmmease merged 1 commit into plotly:master from pb-cdunn:master
Apr 8, 2019
Merged

Fix race conditions #1498

jonmmease merged 1 commit into plotly:master from pb-cdunn:master
Apr 8, 2019

Conversation

@pb-cdunn
Copy link

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

Copy link
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
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.

pb-cdunn reacted with thumbs up emoji

@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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.8.0

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /