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. |
| # 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.
joshmoore
left a comment
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! |
…4358) Zarr v2 retried DirectoryStore's rename because Windows intermittently refuses to replace a destination (#597, fixed by #698). Atomic writes arrived in v3's LocalStore in #3412 without that retry, so the failure is back: _atomic_write's tmp_path.replace(path) raises PermissionError: [WinError 5] Access is denied: '...zarr.<hex>.partial' -> '...zarr.json' and aborts the write. Reported in #3522. _move_with_retry wraps the final move, retrying only the two Windows codes that mean the destination could not be superseded right now. It needs no platform test: off Windows an OSError carries no winerror, so the first attempt either succeeds or raises. The exclusive path is routed through it too but is unaffected by construction -- the FileExistsError it relies on to report an existing node is ERROR_ALREADY_EXISTS (183), which is not in the retried set, so it still propagates on the first attempt. Measured on Windows 11, 4,000 group-attr rewrites (each a replace onto an existing zarr.json): 155-171 raised before, 0 after, for 3.68 s -> 4.08 s of wall clock on a workload that is nothing but replace-onto-existing. In a narrower stdlib-only loop of 20,000 replaces, 475 of 498 recoveries needed only the second attempt and the worst needed the fourth.
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: