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.

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

Make sure we're working with io.js #208

Closed
paulmillr opened this issue Jan 14, 2015 · 14 comments
Closed

Make sure we're working with io.js #208

paulmillr opened this issue Jan 14, 2015 · 14 comments

Comments

@paulmillr
Copy link
Owner

No description provided.

@es128
Copy link
Contributor

es128 commented Jan 14, 2015

Unfortunately it appears there are some issues 😞

(note all tests pass on my machine when using node)

~/Code/chokidar (master●)
λ iojs ./node_modules/.bin/mocha --reporter min

  110 passing (2m)
  10 failing

  1) chokidar fsevents watch individual files should ignore unwatched siblings:
     Uncaught AssertionError: expected spy to have been called with arguments add, /Users/eshanker/Code/chokidar/test-fixtures/add.txt
      at null._onTimeout (/Users/eshanker/Code/chokidar/test.js:278:34)
      at Timer.listOnTimeout (timers.js:90:15)

  2) chokidar fsevents watch glob patterns should correctly watch and emit based on glob input:
     Uncaught AssertionError: expected spy to have been called with arguments add, /Users/eshanker/Code/chokidar/test-fixtures/add.txt
    spy(add, /Users/eshanker/Code/chokidar/test-fixtures/change.txt, {
  atime: Wed Jan 14 2015 09:12:43 GMT-0500 (EST),
  birthtime: Wed Jan 14 2015 09:12:43 GMT-0500 (EST),
  blksize: 4096,
  blocks: 8,
  ctime: Wed Jan 14 2015 09:12:55 GMT-0500 (EST),
  dev: 16777220,
  gid: 1523313980,
  ino: 36714763,
  mode: 33188,
  mtime: Wed Jan 14 2015 09:12:55 GMT-0500 (EST),
  nlink: 1,
  rdev: 0,
  size: 1,
  uid: 987117979
}, undefined, undefined)
      at null._onTimeout (/Users/eshanker/Code/chokidar/test.js:347:32)
      at Timer.listOnTimeout (timers.js:90:15)

  3) chokidar fsevents watch symlinks should watch symlinked files:
     Uncaught AssertionError: expected spy to have been called with arguments change, /Users/eshanker/Code/chokidar/test-fixtures/link.txt
    spy(add, /Users/eshanker/Code/chokidar/test-fixtures/link.txt, {
  atime: Wed Jan 14 2015 09:12:43 GMT-0500 (EST),
  birthtime: Wed Jan 14 2015 09:12:43 GMT-0500 (EST),
  blksize: 4096,
  blocks: 8,
  ctime: Wed Jan 14 2015 09:12:57 GMT-0500 (EST),
  dev: 16777220,
  gid: 1523313980,
  ino: 36714763,
  mode: 33188,
  mtime: Wed Jan 14 2015 09:12:57 GMT-0500 (EST),
  nlink: 1,
  rdev: 0,
  size: 1,
  uid: 987117979
}, undefined, undefined)
      at null._onTimeout (/Users/eshanker/Code/chokidar/test.js:418:32)
      at Timer.listOnTimeout (timers.js:90:15)

  4) chokidar polling watch should emit `add` event when file was added:
     Uncaught AssertionError: expected rawSpy to have been called at least once, but it was never called


  5) chokidar polling watch should emit `addDir` event when directory was added:
     Uncaught AssertionError: expected rawSpy to have been called at least once, but it was never called


  6) chokidar polling watch should emit `change` event when file was changed:
     Uncaught AssertionError: expected rawSpy to have been called at least once, but it was never called


  7) chokidar polling watch should emit `unlink` event when file was removed:
     Uncaught AssertionError: expected rawSpy to have been called at least once, but it was never called


  8) chokidar polling watch should emit `unlinkDir` event when a directory was removed:
     Uncaught AssertionError: expected rawSpy to have been called at least once, but it was never called


  9) chokidar polling watch should emit `unlink` and `add` events when a file is renamed:
     Uncaught AssertionError: expected rawSpy to have been called at least once, but it was never called


  10) chokidar polling watch should notice when a file appears in a new directory:
     Uncaught AssertionError: expected rawSpy to have been called at least once, but it was never called

@es128
Copy link
Contributor

es128 commented Jan 14, 2015

Looks like there are no logical issues, just timing. Node.js versions 0.11 and 0.8 had similar problems. Tweaking the delay settings to resolve.

@es128 es128 closed this as completed in 3a32e61 Jan 14, 2015
@es128
Copy link
Contributor

es128 commented Jan 14, 2015

~/Code/chokidar (master)
λ git rev-parse --short HEAD
3a32e61

~/Code/chokidar (master)
λ iojs ./node_modules/.bin/mocha --reporter min

  120 passing (3m)

@es128
Copy link
Contributor

es128 commented Jan 15, 2015

I just realized that the whole timing issue was actually occurring because every time I tested on another version it was falling back to polling silently. npm rebuild needs to be run before testing on a different version. Still trying to figure out how to build in a way that targets io.js

@es128
Copy link
Contributor

es128 commented Jan 15, 2015

Figured out how to make io.js take over with nvm deactivate. With the recent updates to fsevents, everything is looking good now.

@bpasero
Copy link

bpasero commented Feb 8, 2015

@es128 isn't chokidar still requiring fsevents although io.js is using fsevents under the hood? would it be possible to make the fsevents dependency not be a requirement for native file watching on Mac if running in io.js?

@paulmillr
Copy link
Owner Author

@bpasero I don't see how io.js exposes any api which is "good enough" / generic enough for chokidar to use.

@bpasero
Copy link

bpasero commented Feb 8, 2015

@paulmillr right, I was hoping that their fs.watch() is good enough for chokidar. They recently added support for recursive watching on MacOS leveraging FSEvents.

Did you guys ever thought about contributing your work back to io.js? I think they are much more open to contributions compared to node.js.

@es128
Copy link
Contributor

es128 commented Feb 8, 2015

@bpasero feel free to set useFsEvents: false, usePolling: false and let us know how it goes.

Setting up chokidar to use the recursive: true option in the environments where it is available would effectively be like adding a fourth watch mode as long as we want to keep supporting node <= 0.10, and that's adding much more complexity than I want to manage for little, if any, benefit. From what I saw in 0.11 I expect there will still be lots of issues with fs.watch for a long time to come anyway.

As for contributing back - not opposed to it at all of course, but chokidar continues to make more sense as a userspace module.

@bpasero
Copy link

bpasero commented Feb 10, 2015

@es128 my understanding is that chokidar on Windows is using fs.watch(). are you saying that even if there was native support for recursive watching (see joyent/libuv#1473), chokidar would not adopt this?

My concern with the current way on Windows is that watching large directories causes high CPU/Mem load and potentially locks folders due to the recursive fs.watch() calls.

@es128
Copy link
Contributor

es128 commented Feb 10, 2015

That would be a different situation than what you first started asking on Mac, as it may make a meaningful performance impact. That's assuming whatever they do is fundamentally different and more performant than what chokidar already does. And still, it would be very tricky to juggle all the different modes targeting different platforms + node/io variants.

@bpasero
Copy link

bpasero commented Feb 10, 2015

@es128 yeah, sorry I am jumping a bit around. I am looking into allowing to enable the native recurse option on Windows for ReadDirectoryChangesW and while I get it to work, I am likely still suffering from all the issues around how libuv handles the event. For example, I am not getting a delete event when deleting a file, rather a "rename: null" event which is not helpful either.

I understand that chokidar has code in place to detect these issues and prevent them. Would it help at all if the native recurse flag was enabled given the issues with fs.watch() in general? In other words, how does Chokidar solves these issues. Are you keeping a list of children per folder and compare the contents on such an event?

@es128
Copy link
Contributor

es128 commented Feb 10, 2015

I haven't really played with it, but afaict the native recursive flag just means the ability to open less watcher instances at the high level. I have no idea whether there will be any performance benefit underneath like there is with fsevents.

It would require a very substantial refactor to the fs.watch handler code to leverage the recursive option, it's not something we could just turn on.

Are you keeping a list of children per folder and compare the contents on such an event?

yes

@bpasero
Copy link

bpasero commented Feb 11, 2015

@es128 I did a test comparing chokidar with fs.watch() using the recursive flag on Windows and there are major differences in terms of performance for larger folders. I understand that chokidar is doing some extra magic to get the right set of events, but imho the performance on Windows is unacceptable. I filed #228

With fs.watch() I see no CPU/mem increase whatsoever even for my large Desktop and file events bubble in instantly after startup (as opposed to chokidar - see my bug for it).

paulmillr added a commit that referenced this issue Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants