Make DirectoryStore __setitem__ resilient against antivirus file locking #698
Conversation
Codecov Report
@@ Coverage Diff @@
## master #698 +/- ##
=======================================
Coverage 99.94% 99.94%
=======================================
Files 28 28
Lines 10237 10266 +29
=======================================
+ Hits 10231 10260 +29
Misses 6 6
|
|
Hello @ericgyounkin! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-03-04 13:14:43 UTC |
|
Not sure about these meta.py Linting error messages in Linux Testing / build (3.8). Am I missing something? |
|
regarding adding a test, perhaps something like: |
|
These errors others have seen: so not related to your PR. @Carreau have you seen these elsewhere? |
|
@joshmoore Thats great, thanks! I hadn't seen pytest raises used before, so that makes a lot of sense. How would I use your commit in my PR? |
|
Ignore last comment, pulled from @joshmoore commit and updated PR |
|
@joshmoore I'm a little confused about the status of this pull request, is there anything I should do further? I see that these checks now pass after your changes. Thanks for your work on this. |
|
Hey @ericgyounkin. Apologies. I think I pushed the last commit with my fingers crossed and then moved on to something else while waiting on the tests to go green. Let me merge in master and make sure everything is green, but I assume there's nothing else from your side. Thanks for hanging in there! |
|
Thanks @joshmoore! Do you know how the release schedule works? When the next release is planned? Just planning for the future of my app. |
|
Trying again to get things green after #705. |
zarr/storage.py
Outdated
| # move temporary file into place; | ||
| # make several attempts at writing the temporary file to get past | ||
| # potential antivirus file locking issues | ||
| retry_call(os.replace, (temp_path, file_path)) |
There was a problem hiding this comment.
@ericgyounkin : just realizing (since I have never been able to test this in anger) doesn't PermissionError need listing here? Otherwise, no exceptions are going to be caught.
There was a problem hiding this comment.
@joshmoore Yes, my mistake. Just added it.
There was a problem hiding this comment.
It's a bit disingenuous for me to review this considering I submitted the function that's being used, but at least there have been two sets of eyes on it. If anyone has issues with the method signature, there's still some time to clean it up before the release of 2.7.0. Otherwise, we'll see how this fairs in the wild.
|
Thanks for hanging in there, @ericgyounkin! |
|
@joshmoore Thank you for all your work, big Zarr fan! |
See #597
Enterprise and government computers generally have always-on realtime file scanning antivirus software that causes problems with any system that relies on creation/deletion of lots of small files. We have seen this with SQLite as well, with the temp files it uses. The fix I'm proposing here is to simply try a few times and wait inbetween tries for the antivirus software to release the file lock. I've been running with this fix for a while, and I can now use Zarr on government machines. Hugely helpful. I've seen PermissionErrors floating around the gitter chat and in stackoverflow, so I think this fix is needed for others as well.
The 0.1 sec delay was empirically determined, I've found that the vast majority of writes that generate a PermissionError on the first attempt will work on the second.
Not sure how to build a test for this.
TODO: