Fix os.makedirs issue in multithreading (#2039) #2093
Conversation
|
In Python 3.5 on Linux, FileExistsError has the errno attribute. So you can simplify the logic to always catch OSError and unconditionally access err.errno attribute. Test performed in Ipython: n [3]: try: os.makedirs("/tmp")
...: except Exception as e:
...: a = e
...:
In [4]: a
Out[4]: FileExistsError(17, 'File exists')
In [5]: a.errno
Out[5]: 17
|
|
Not sure if this is the same thing, but there's a similar compatibility-hack here: https://github.com/spotify/luigi/blob/master/luigi/lock.py#L125-L129 |
|
Yes, that hack should also be changed to check errno, to correctly propagate other OSError errors in Python 2. |
FileExistsError has the errno attribute.
|
FileExistsError has a errno attribute. |
luigi/local_target.py
Outdated
| try: | ||
| FileNotExistsError | ||
| except NameError: | ||
| FileNotExistsError = OSError |
There was a problem hiding this comment.
To reduce complexity, please use OSError on Python3 as well.
luigi/local_target.py
Outdated
| try: | ||
| os.makedirs(path) | ||
| except FileNotExistsError as err: | ||
| # somebody already created the path |
There was a problem hiding this comment.
This comment is very helpful explaining why we can ignore the error. Thanks for adding it.
| os.makedirs(path) | ||
| except OSError as err: | ||
| # somebody already created the path | ||
| if err.errno != errno.EEXIST: |
There was a problem hiding this comment.
Typo on EEXIST. Should be EXIST
There was a problem hiding this comment.
I think it's correct. https://docs.python.org/3.6/library/errno.html#errno.EEXIST
There was a problem hiding this comment.
Well I didn't expect that. I recant.
|
If no one else reviews, i'll merge this week. |
|
Sorry for such delay @elnikkis ! Thanks for your first Luigi contribution! |
Description
There is a problem that os.makedirs is not atomic in multithreading.
Motivation and Context
This problem is reported in issue #2039.
Since exist_ok was added in Python 3.2 or later, it cannot be used in Python 2.7, so we need to check OSError.errno that we caught.
But in Python 3.x, os.makedirs raise FileExistsError instead of OSError.
FileExistsError is a subclass of OSError but does not have errno attribute.
Have you tested this? If so, how?
In my environment, it seems to work well.