Fix for a race condition on Linux #1228
Conversation
In two of the tests there were unlink() calls at the end of an asynchronously executed function. These calls would sometimes end up executing only after the test has finished and after cleanup, leading to them erroring out due to target files no longer around. These failures would cause exceptions reported in other, unrelated tests. Since the tests containing them work just fine without the unlink calls (as is evident by the fact that tests passed even if they executed after finishing the case) simply removing them fixes the problem and makes the test suite reliable.
|
good stuff! |
|
Looks like the fix is breaking some tests on non-linux platforms. Do you think it's a good idea to conditionally do this on Linux only? Or do you think the failures are unrelated? I don't have access to either platform so I can't say for sure, but the test suite is quite brittle. The one fix I included here seemed to be enough to get it to pass robustly on my system, but there might be more dragons there. |
|
The nodefs-handler is not used on macos, so i'm not sure how to fix it. Maybe in the same way in fsevents-handler. |
|
That's a good point, I forgot. In this case I think it's fair to say that these test failures must be unrelated. |
Sometimes, when another process just happened to create a file when we were setting up a watch, the new file wouldn't be noticed. To prevent the event not being generated in this case, schedule a directory rescan immediately after setting up a watch. Fixes paulmillr#1112
dividedmind
force-pushed
the
fix/new-file-new-directory-race-on-linux
branch
from
a2ea88c
to
95bc9cd
Compare
|
I changed it to only do the readdir if polling is enabled and added a workaround for a Node 8 bug. I wanted to take a look at more Node 8 test problems, but then I realized it's over two years past EOL and it's probably not worth the trouble. I verified tests pass robustly on Node >= 14 on Linux. The MacOS and Windows test failures seem unrelated, so I think this is good to merge. |
paulmillr
force-pushed
the
master
branch
2 times, most recently
from
368cb02
to
7c50e25
Compare
Sometimes, when another process just happened to create a file
when we were setting up a watch, the new file wouldn't be noticed.
To prevent the event not being generated in this case, schedule
a directory rescan immediately after setting up a watch.
Fixes #1112
Also, another commit here to fix brittle tests. It might be related to #923