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.

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

Back filesystem watchers with @atom/watcher #16124

Merged
merged 2 commits into from Feb 14, 2018

Conversation

Projects
None yet
4 participants
@smashwilson
Member

smashwilson commented Nov 7, 2017

Add a new "experimental" setting for core.fileSystemWatcher that backs watchPath with @atom/watcher. There should be no changes for API consumers.

⚠️ There's a reason this is marked "experimental!" Our intention is to not merge this PR until I'm back from pending paternity leave, then let it percolate its way through master, beta, and stable before we consider flipping the feature flag on for everyone. In the meantime, if you want to see if this fixes a filesystem watching issue that you're having, you'll need to build Atom from this branch. ⚠️

Remaining Work

These are known issues in @atom/watcher that I'd like to address before this merges. Mostly, these avoid regressing on some edge cases that have been reported with the current watcher.

Features and Limitations

In its current state, the experimental watcher:

  • Uses one or two threads for all watcher threads, rather than two threads per watcher.
  • Fails gracefully to polling when (a) more than 451 watch roots are created on MacOS; (b) ReadDirectoryChangesW() reports that a directory does not support watching on Windows; or (c) the maximum number of inotify watch descriptors is exceeded on Linux.
  • Can log a bunch of diagnostic information to track down edge cases.
  • Scales to thousands of watch roots containing tens of thousands of files.
  • Watch directories recursively or non-recursively.
  • Watch individual files without polling.

It cannot currently:

  • Follow symlinks.
  • Explicitly include or exclude subpaths of a watched directory.
  • Receive notifications on certain network shares.
  • Report drive mount and unmount events.

Test Plan

Before we merge, I'd like to verify that we handle or fail gracefully in a number of trickier situations. In each scenario, if we:

  • 🔴 crash in native code or lock up, I would consider that a blocker for merging this.
  • ⚠️ throw a JavaScript stack or silently fail to deliver any events, I'd consider merging anyway, with an issue filed against @atom/watcher.

These are some of the cases I've thought to try:

  • Open a project directory on a network drive.
    • MacOS: sshfs
    • MacOS: Samba
    • Windows: native Windows-to-Windows network share
    • Windows: Samba
    • Linux: sshfs
    • Linux: Samba
  • Open a project directory from a flash drive.
    • MacOS
    • Windows
    • Linux
  • Unmount a filesystem that contains an open project directory.
    • MacOS
    • Windows
    • Linux
  • Mount a new filesystem beneath a watched directory.
    • MacOS
    • Linux
  • Cloning a large git repository beneath a watched directory.
    • MacOS
    • Windows
    • Linux

To Opt In

If you want to give this a try before it merges, clone and build Atom from this branch:

git clone https://github.com/atom/atom.git --branch aw-watcher
cd atom
script/build --install

Launch Atom and set core.fileSystemWatcher to "experimental" in your core settings.

@smashwilson

This comment has been minimized.

Member

smashwilson commented Nov 15, 2017

/cc @ungb I'd ❤️ your input on the test plan in the PR description once you're back in the office 😄 In particular, I have no idea what kinds of network drives we should test, or, like, how we should do that. NFS? Samba? sshfs... ?

/cc @rsese @lee-dohm @50Wliu in case any of you have seen any other filesystem-related issues fly by recently that we should verify this doesn't regress.

/cc @Arcanemagus who has been helping a bunch with the @atom/watcher work. 🙇 You're welcome to check this out in Atom proper, although unfortunately I don't think your NAS will work yet (I haven't done anything to get it out of the "silently deliver no events" category.)

@smashwilson smashwilson referenced this pull request Nov 16, 2017

Open

(Windows) Unknown and unexpected results #93

1 of 1 task complete
@lee-dohm

This comment has been minimized.

Member

lee-dohm commented Nov 16, 2017

@smashwilson I feel like for network support we should test:

  1. Native Windows-to-Windows network shares
  2. sshfs
  3. Samba

in that priority order. Also, we should test them through a connection with restricted network throughput and latency generator to see what the limits are for our implementation. (In other words, will sshfs over a high-latency 3G cellular connection work as expected?)

@50Wliu

This comment has been minimized.

Member

50Wliu commented Nov 20, 2017

I'll try to run through flash drive and cloning a large git repository on Windows soon.

@50Wliu

This comment has been minimized.

Member

50Wliu commented Jan 26, 2018

Got around to checking the scenarios I mentioned above and everything seems all right 👍. Decent performance, no crashes or anything.

@smashwilson

This comment has been minimized.

Member

smashwilson commented Feb 5, 2018

Got around to checking the scenarios I mentioned above and everything seems all right 👍. Decent performance, no crashes or anything.

Excellent, thank you! You're on Windows, right?

@smashwilson

This comment has been minimized.

Member

smashwilson commented Feb 5, 2018

On reflection I'm likely going to punt on atom/watcher#70 until after this is merged. It isn't entirely clear to me what the right thing to do is there, and I don't want to hold this up when a better solution might not involve @atom/watcher at all. What's more, nsfw also doesn't handle this well, so we're no worse off than what's currently shipped.

I would like to tackle atom/watcher#19 though.

@ungb: Do we have a samba share or an sshfs mount already available somewhere that we could use for network drive tests? It might be nice to have those available for general testing in any case 🤔

@50Wliu

This comment has been minimized.

Member

50Wliu commented Feb 5, 2018

You're on Windows, right?

Yep.

@smashwilson

This comment has been minimized.

Member

smashwilson commented Feb 5, 2018

on Windows

... I can read, really

@ungb

This comment has been minimized.

Contributor

ungb commented Feb 5, 2018

@ungb: Do we have a samba share or an sshfs mount already available somewhere that we could use for network drive tests? It might be nice to have those available for general testing in any case 🤔

@smashwilson we do have something for sshfs, let me look into samba share.

@smashwilson

This comment has been minimized.

Member

smashwilson commented Feb 14, 2018

I saw a crash when you disconnect a Samba share on Windows while it contains a watched directory. Tracking it in atom/watcher#118.

image

smashwilson added some commits Nov 7, 2017

@smashwilson smashwilson merged commit 9f8370c into master Feb 14, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@smashwilson smashwilson deleted the aw-watcher branch Feb 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment