#272 Fix race condition in DirectoryStore #318
Conversation
zarr/storage.py
Outdated
| @@ -758,6 +758,9 @@ def __setitem__(self, key, value): | |||
| if not os.path.exists(dir_path): | |||
| try: | |||
| os.makedirs(dir_path) | |||
| except OSError: | |||
| if not os.path.isdir(dir_path): | |||
| raise KeyError(key) | |||
| except Exception: | |||
| raise KeyError(key) | |||
There was a problem hiding this comment.
Hmm...so this except case doesn't catch it?
There was a problem hiding this comment.
The except Exception block was catching it before, but it was just throwing KeyError instead--so my code would still error out. The block that I added only raises KeyError if the folder doesn't exist already. This way, workers can go on with their business if the folder already exists.
There was a problem hiding this comment.
Should we then drop the except Exception entirely and only use except OSError? It seems bad to mask other errors by raising KeyError instead.
There was a problem hiding this comment.
I think that's a good idea. I wasn't sure about that part so I left it, but I'm not sure what other exceptions you can even expect from os.makedirs. If you got OSError because of permissions or something else that prevented making the folder, then the block that I added would raise KeyError anyway.
There was a problem hiding this comment.
Good point. We can improve this a bit then by checking the errno of the OSError. Namely if we find e.errno == errno.EEXIST, then we know this is a FileExistsError. If it's any other errno, we can just re-raise the exception as is.
ref: https://docs.python.org/3/library/exceptions.html#FileExistsError
ref: https://docs.python.org/3/library/errno.html#errno.EEXIST
There was a problem hiding this comment.
Have taken the liberty of pushing a commit to your branch using the errno bit. Please let me know what you think.
There was a problem hiding this comment.
I wasn't sure about that part so I left it...
That makes sense. Honestly this is an orthogonal issue. So we need not address it here. Will raise an issue to follow-up on this after.
There was a problem hiding this comment.
Went ahead and dropped the generic exception case as it didn't seem to be covered. Happy to revisit if we figure out it was needed for some reason.
On Python 3, we would only want to catch the race case here by checking for the `FileExistsError` and suppressing raising for that case. However there is no `FileExistsError` on Python 2. Only `OSError` exists on Python 2/3. However that catches some other errors that we might not want to suppress here. So we check the `errno` of the `OSError` instance to verify it is caused by an existing file/directory represented by the `EEXIST` error number. This amounts to catching the `FileExistsError` in a Python 2/3 friendly way. All other `OSErrors` we raise as before.
|
@alimanfoo, when you have a chance, could you please give this a quick look? Shouldn't take too long I hope. :) |
As we now properly handle the existing file case properly, go ahead and drop the generic exception case. This should now raise some errors that were hidden previously.
|
Thanks @jmswaney! |
|
Have added the release note to PR ( #409 ). If there are any other issues, please let us know and we can follow-up on them. |
Multiple workers may attempt to create subfolders when using
NestedDirectoryStore, leading toFileExistsErrorraised. This fix catchesOSError(for python 2 support) and checks if the directory already exists--if it does, then no exception is raised.Fixes #272