win,test: try again if file unlink fails #284
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
piscisaureus
force-pushed
the
test-fix-unlink
branch
from
cba0c03 to
b05d30a
Compare
piscisaureus
changed the title
| except OSError, e: | ||
| # On Windows unlink() fails if another process has the file open. | ||
| # Yield and try again, and it'll succeed. | ||
| if os.name == 'nt' and e.errno == errno.EACCES: |
There was a problem hiding this comment.
sys.platform == 'win32' is slightly more idiomatic. Maybe the comment could mention that 'another process' means a virus scanner or an indexer because the only two calls to CheckedUnlink() are for temp files. An unwitting reader could easily assume that the loop is not in fact necessary.
piscisaureus
force-pushed
the
test-fix-unlink
branch
from
b05d30a to
44644a1
Compare
Now that parallel tests are enabled, the test runner spits out a ton of 'access denied' errors while running the tests. These happen because a virus scanner or the indexing service temporarily open the file after it has been updated, and the test runner tries to unlink() them at the same time. This patch resolves this issue by attempting to unlink the file until it succeeds. PR-URL: nodejs#284 Reviewed-by: Ben Noordhuis <[email protected]>
piscisaureus
force-pushed
the
test-fix-unlink
branch
from
44644a1 to
b57e9a9
Compare
|
LGTM even if makes me feel a little squeamish. |
This was referenced Nov 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now that parallel tests are enabled, the test runner spits out a ton of
'access denied' errors while running the tests. These happen because a
virus scanner or the indexing service temporarily open the file after it
has been updated, and the test runner tries to unlink() them at the same
time.
This patch resolves this issue by attempting to unlink the file until it
succeeds.
R=@bnoordhuis