Fix issue #49: Retry rmdirSync on Windows for up to 1 second if files still exist.#179
Conversation
|
I have an idea on how to make a test for this using https://github.com/cjohansen/Sinon.JS by stubbing |
|
Looks good, I don't think we need to a specific platform test for this.. I need a second pair of eyes though - @Schoonology ? |
|
Looking over that issue again, I wouldn't retry on |
|
I've left all the comments I can think of for now. Thoughts, rebuttals, etc.? Though this is a relatively small amount of code, I feel like |
|
@Schoonology I fixed the style issues you pointed out and re-pushed. Regarding EPERM: It was one of the errors I was getting for which retry helped. For the EPERM case, it could be an antivirus related issue. I don't think 1 second of retrying is that much of a big deal if the file is actually in use, before the error is re-thrown. Regarding checking for win32: on other plaforms the files should disappear even if they're in use, it's only Windows that locks files from being deleted under such circumstances. If an error occurs on Linux for instance, it's probably because of permissions, not because of quirks such as this one. (at least - I couldn't reproduce this error on Linux and MacOSX, it works fine there) |
|
There's one more issue here: In my usage, after doing a rm -rf, I'm recreating the same directory I deleted. This sometimes results in an error in mkdir because the directory didn't disappear yet. There needs to be an additional check on Windows whether the directory is gone before returning from the function. There won't be any error in this case in rmdirSync and nothing logged, but it just takes one extra milisecond for the directory to actually disappear from the file system. To reiterate, shelljs is being used as part of a test suite for a library I'm working on and this was the final fix to all of the shelljs related problems I was getting running this test suite of 100+ tests on Windows. |
andreialecu
force-pushed
the
fix-rmrfwin32
branch
from
11688e8 to
6c2d854
Compare
andreialecu
force-pushed
the
fix-rmrfwin32
branch
from
6c2d854 to
57a3725
Compare
|
@Schoonology need your input on https://github.com/arturadib/shelljs/pull/179/files#diff-d0eda960c284276e6b0b269826fec539R56 I thought this was the simplest, I don't think that line will ever throw on non-Windows, but if it ever does, I think it's a good thing. I assume the calling application would want to know an allegedly successful call to remove a directory somehow still left the directory behind. Thoughts? |
src/rm.js
Outdated
There was a problem hiding this comment.
I think this is fine, though since it's potentially user-facing, I'd like to invest a little more time in the thrown Error:
- Throw an actual
Error. No base class needed. EEXISTis specific to creating files. Could we use a more generic error likeEAGAINor evenEOTHER?- A good message.
@arturadib - Do we have a precedent for these kinds of errors?
There was a problem hiding this comment.
I think we use common.error() for error messages
There was a problem hiding this comment.
Let me know what you want to do about it and I'll change it.
@Schoonology Regarding 1 and 2: I'm not sure how you would concatenate a good message with an actual Error without creating a new Error class. Could you clarify?
@arturadib The code flow goes to the outer catch which does use common.error(). That throw there is just to give it a chance to retry. It's handled by the inner catch in the if. If the error is re-thrown, the outer catch handles it normally.
There was a problem hiding this comment.
@andreialecu - I completely missed that outer catch. Thanks for being patient with me, and pointing that out. That solves 1 and 3, and I'd just replace 2 with EAGAIN, because trying again may succeed, whereas EEXIST is meant to indicate that creating a new file failed because one already exists, and the user should not try again until that's resolved.
Does that make sense?
There was a problem hiding this comment.
Hey @Schoonology, I changed it to EAGAIN. Check it out now.
|
I'm good with this, and I think @arturadib's concerns are satisfied with that outer Squash it and |
.
|
bump. It'd be nice if this could get merged, I'm running into it a lot on Windows... |
Fix issue #49: Retry rmdirSync on Windows for up to 1 second if files still exist.
|
changes published to version 0.5.1, thanks everyone! |
|
Coool ! 👍 |
5 participants
See issue #49