FIX: subtle race condition in cbook.mkdirs#5224
FIX: subtle race condition in cbook.mkdirs#5224jenshnielsen merged 3 commits intomatplotlib:v1.5.xfrom
Conversation
tacaswell
modified the milestones:
next point release (1.5.0),
next bug fix release (2.0.1)
|
closes #5225 |
|
Thanks. Looks good to me except that I wonder why the Python2.7 code calls |
|
Good point, simplified. |
lib/matplotlib/cbook.py
Outdated
There was a problem hiding this comment.
(nit): I know it's equivalent, but I think I'd prefer to see the Python 2 stuff in an else clause so it's more obvious it only runs on Python 2.
There was a problem hiding this comment.
done. That make lots of sense and will make ripping out the LPY stuff easier.
lib/matplotlib/cbook.py
Outdated
There was a problem hiding this comment.
Do we know that this works with multiple processes? I thought the whole point of exist_ok in Python3 was to handle the case of the directory pre-existing, and I thought the initial attempt at rewriting the code to look at each leaf separately was to work around the multi-process problem. Isn't this code now back to how it was before the change to protect from multiple processes attempting to create the directory? My previous comment was that os.makedirs looked like it should just be os.mkdir as the code was only ever creating a directory at a time.
There was a problem hiding this comment.
I think we have now delegated that problem down to cpython?
|
The reimplementation of |
|
@tacaswell for some reason I can't see the comment I just got in email on Github. I think I'm saying that CPython 2.7 may not handle the multi-process race condition properly and simply calling |
|
https://github.com/python/cpython/blob/master/Lib/os.py#L216 <- current implementation of https://github.com/python/cpython/blob/2.7/Lib/os.py#L136 <- implementation for 2.7 which also looks like it handles this race condition properly. It looks like 2.5 (https://github.com/python/cpython/blob/2.5/Lib/os.py#L150) is where that checking first shows up (2.4: https://github.com/python/cpython/blob/2.4/Lib/os.py#L143) you have to click the 'show outdated diff' link up the page a bit to see the comment. |
|
Thanks. I'm happy now 😄 👍 |
FIX: subtle race condition in cbook.mkdirs
5 participants
attn @timj