-
Notifications
You must be signed in to change notification settings - Fork 653
Conversation
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):
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
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) \ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/hanlde/handle/
Last round of comments. If you fix those then it LGTM. |
Thanks, landed with fixes in cd2794c. Backporting to v0.10 now. |
Nice work @indutny ! |
Thanks :) Waiting for merging backport before closing this issue: #896 |
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? |
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
toreturn false and write following message to stderr:
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