Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

linux: fix missing events #127

Merged
merged 3 commits into from
Mar 12, 2021
Merged

linux: fix missing events #127

merged 3 commits into from
Mar 12, 2021

Conversation

paul-marechal
Copy link
Contributor

@paul-marechal paul-marechal commented Feb 16, 2021 •
edited
Loading

See individual commit messages for more details.

NSFW's linux implementation suffers from a small window of opportunity where if a folder is created, it will go totally undetected. See #60 (comment). The first commit addresses that issue by allocating the inotify watcher before doing scandir when recursively building the node tree.

The second commit tries to emit creation events for nested files and folders of newly added sub-trees. It was a bit up to luck before if NSFW would report something from inside that nested tree, but now it should always properly dive inside and report all files.

Fixes #60

@paul-marechal
Copy link
Contributor Author

@implausible heads up. This is something we wanted to fix for Theia, but it should affect GitKraken as well?

@paul-marechal paul-marechal force-pushed the mp/linux-event-fix branch 2 times, most recently from 47a0ea4 to 5e08354 Compare February 17, 2021 15:25
@paul-marechal paul-marechal changed the title fix missing events linux: fix missing events Feb 17, 2021
@paul-marechal
yarn upgrade
2dc1726 
Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
@paul-marechal
attach inotify watcher before scandir
928d9e1 
There is a window where some created folders might get ignored
completely. It happens between the time where the tree is created and
the time where the tree is iterated over to allocate watchers.

This commit hoist the inotify watching before doing scandir. This way
there is no window where a directory might be created after the initial
scandir and missed, as the inotify watcher should pick it up.

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
@paul-marechal
Copy link
Contributor Author

I added a yarn upgrade commit which seems to fix CI.

@implausible
Copy link
Contributor

This is awesome. Quick look, I'm a bit concerned that the testing apparatus is strongly tied to the implementation details of the libraries codebase. I will come back with a suggestion hopefully this week.

@implausible
Copy link
Contributor

I mean... leveraging conditional compilation to set the test mechanism might be better? I wonder if we set an environment variable if we could pick it up as a define in gyp land. Then only in the test run does this codepath exist.

@paul-marechal
Copy link
Contributor Author

leveraging conditional compilation to set the test mechanism might be better?

Good idea, I'll do that!

@paul-marechal paul-marechal force-pushed the mp/linux-event-fix branch 2 times, most recently from 6eadcd9 to 1ee52c2 Compare February 27, 2021 01:04
paul-marechal
paul-marechal commented Feb 27, 2021
View reviewed changes
Copy link
Contributor Author

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@implausible I implemented the build-time macro like suggested.

@paul-marechal paul-marechal force-pushed the mp/linux-event-fix branch 4 times, most recently from fb78f4c to ef061c9 Compare February 27, 2021 19:57
@paul-marechal
Copy link
Contributor Author

I exposed the Napi NSFW object through require('nsfw')._native, this way I can access fields defined from c++ to check if the build includes the sleep code path.

@paul-marechal
Copy link
Contributor Author

CI is getting stuck on some test but I can't reproduce on my Linux machine. Can someone else confirm?

@paul-marechal paul-marechal force-pushed the mp/linux-event-fix branch 3 times, most recently from 7094910 to 94163de Compare March 3, 2021 22:25
@paul-marechal
Copy link
Contributor Author

Added a --exit flag when invoking mocha because it was hanging despite the event loop looking empty...

@implausible
Copy link
Contributor

implausible commented Mar 8, 2021 •
edited
Loading

Added a --exit flag when invoking mocha because it was hanging despite the event loop looking empty...

That's usually a sign that the native module did not cleanly shutdown, and is usually bar to investigate and fix. Here are the things that can cause the test suite not to exit:

  1. timeout or interval handle is open
  2. uv_async_t handle is open

implausible
implausible suggested changes Mar 8, 2021
View reviewed changes
js/spec/nested-file-test.js Outdated Show resolved Hide resolved
js/spec/nested-file-test.js Outdated
await fse.mkdirp(directory);
await fse.close(await fse.open(filePath, 'w'));
await Promise.race([
sleep(folders.length * 500 * 1.5),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the timer that's holding on for dear life? Maybe we should cancel the timeout if it loses the race.

Copy link
Contributor Author

@paul-marechal paul-marechal Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked with why-is-node-running after the test and the event loop was empty. Also that sleep is not going on for an hour, which is the time it took for CI to be cancelled... Lastly is it possible to cancel promisified timeouts? I didn't bother for the aforementioned reasons.

Copy link
Contributor Author

@paul-marechal paul-marechal Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what confuses even more is that on two different Linux machines the test runs fine, the issue only happens with GitHub Actions for some reason...

src/linux/InotifyTree.cpp Show resolved Hide resolved
@paul-marechal
emit CREATED event for new directories
f34d5b1 
Most clients expect every new file from a new tree being added in the
watched directory to be reported. This is currently not the case if the
tree appears too quickly on disk.

This commit propagates a callback down the recursive tree building to
make sure that all files added are being reported as created to clients.

I also added an environment variable check for use when testing:
`NSFW_TEST_SLOW`. When set to `1` nsfw will artificially sleep before
crawling folders on the fs. This is to simulate the case when nsfw is
late to the party and sub-folders were created before we had time to
allocate inotify watchers. In such a case, we need to manually emit
CREATED events for new files and folders discovered by scandir.

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
@paul-marechal
Copy link
Contributor Author

@implausible I inlined the test and added the IN_ONLYDIR when creating the inotify watchers.

@implausible implausible merged commit 94e83f0 into Axosoft:master Mar 12, 2021
@implausible
Copy link
Contributor

Thanks 😄

@paul-marechal paul-marechal deleted the mp/linux-event-fix branch March 12, 2021 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@implausible implausible implausible requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mkdir -p are not correctly reported on Linux
2 participants
@paul-marechal @implausible