Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

fsevents: use shared FSEventStream #894

Closed
wants to merge 3 commits into from
Closed

fsevents: use shared FSEventStream #894

wants to merge 3 commits into from

Conversation

indutny
Copy link
Contributor

@indutny indutny commented Aug 21, 2013

It seems that number of simultaneously opened FSEventStreams is limited
on OSX (i.e. you can have only fixed number of them on one running
system), getting past through this limit will cause FSEventStreamCreate to
return false and write following message to stderr:

(CarbonCore.framework) FSEventStreamStart: register_with_server: ERROR:
f2d_register_rpc() => (null) (-21)

To prevent this, we must use only one shared FSEventStream with a paths for
all uv_fsevent_t handles, and then filter out events for each handle
using this paths again.

See nodejs/node-v0.x-archive#5463

@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit indutny/libuv@12852ba has the following error(s):

  • Commit message line too long: 4
  • Commit message line too long: 7
  • Commit message line too long: 10

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

It seems that number of simultaneously opened FSEventStreams is
limited on OSX (i.e. you can have only fixed number of them on
one running system), getting past through this limit will cause
`FSEventStreamCreate` to return false and write following message
to stderr:

    (CarbonCore.framework) FSEventStreamStart: register_with_server:
    ERROR: f2d_register_rpc() => (null) (-21)

To prevent this, we must use only one shared FSEventStream with a
paths for all uv_fsevent_t handles, and then filter out events for
each handle using this paths again.

See nodejs/node-v0.x-archive#5463
@indutny
Copy link
Contributor Author

indutny commented Aug 21, 2013

Should be ABI compatible with v0.10 if I hadn't messed with anything :)

@@ -72,26 +100,21 @@ static void uv__cf_loop_signal(uv_loop_t* loop,

#define UV__FSEVENTS_WALK(handle, block) \
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're here, the name of this macro is somewhat poorly chosen. 'Walk' implies it just iterates but it also mutates it. Can you address that in a follow-up commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, anything else to fix?

It isn't really walking through events, because it changes (resets)
them.

Per @bnoordhuis and @tjfontaine suggestion.
path = paths[i];
len = strlen(path);

/* Filter out paths that are outside hanlde's request */
Copy link
Contributor

Choose a reason for hiding this comment

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

s/hanlde/handle/

@bnoordhuis
Copy link
Contributor

Last round of comments. If you fix those then it LGTM.

@indutny
Copy link
Contributor Author

indutny commented Aug 22, 2013

Thanks, landed with fixes in cd2794c. Backporting to v0.10 now.

@piscisaureus
Copy link

Nice work @indutny !

@indutny
Copy link
Contributor Author

indutny commented Aug 22, 2013

Thanks :)

Waiting for merging backport before closing this issue: #896

@indutny indutny closed this Aug 22, 2013
@indutny indutny deleted the fix/fseventstream-duplication branch August 22, 2013 14:42
@siebeneicher
Copy link

Hey guys, I still get this error using grunt-watch on mac os. Nodejs is up to date (0.10.26). Does this fix practically fixed the mentioned error in the inital comment? indutny?
Short reply very much appreciated! THanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants