-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 symlinking race condition #2765
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, not sure on the best way around this. I guess it depends on what is creating the race condition -- multiple processes/threads creating these paths at the same time?
@@ -1,3 +1,4 @@ | |||
import errno |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this was brought in for this PR, it isn't used.
readthedocs/core/utils/__init__.py
Outdated
""" | ||
Makedirs has an issue where it has a race condition around | ||
checking for a directory and then creating it. | ||
This catches the exception in this case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The failure case isn't clear, perhaps expand on what this is protecting against.
readthedocs/core/utils/__init__.py
Outdated
|
||
try: | ||
os.makedirs(directory_name) | ||
except OSError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this strictly catching makedirs
created dirs that already exist? It seems that OSError
would be thrown in a number of cases that we might care about.
I wonder if this is something we should have locking around?
I think we're hitting a case where multiple builds or celery runs on the webs
are causing a race condition in the symlinking code.
I'm guessing we'll still be hitting issues until we build locking code around this,
because it's pretty stateful. This is not a great fix, but at least stops the builds failing in these cases, I believe.