{"action":"created","issue":{"url":"https://api.github.com/repos/nodejs/nan/issues/284","labels_url":"https://api.github.com/repos/nodejs/nan/issues/284/labels{/name}","comments_url":"https://api.github.com/repos/nodejs/nan/issues/284/comments","events_url":"https://api.github.com/repos/nodejs/nan/issues/284/events","html_url":"https://github.com/nodejs/nan/issues/284","id":58089948,"number":284,"title":"MakeCallback is dangerous, not only or best way of calling callback; provide direct method","user":{"login":"metamatt","id":313893,"avatar_url":"https://avatars.githubusercontent.com/u/313893?v=3","gravatar_id":"","url":"https://api.github.com/users/metamatt","html_url":"https://github.com/metamatt","followers_url":"https://api.github.com/users/metamatt/followers","following_url":"https://api.github.com/users/metamatt/following{/other_user}","gists_url":"https://api.github.com/users/metamatt/gists{/gist_id}","starred_url":"https://api.github.com/users/metamatt/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/metamatt/subscriptions","organizations_url":"https://api.github.com/users/metamatt/orgs","repos_url":"https://api.github.com/users/metamatt/repos","events_url":"https://api.github.com/users/metamatt/events{/privacy}","received_events_url":"https://api.github.com/users/metamatt/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/nodejs/nan/labels/bug","name":"bug","color":"fc2929"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":53,"created_at":"2015-02-18T16:23:11Z","updated_at":"2015-11-16T16:58:56Z","closed_at":null,"body":"NodeJS lets you invoke a \"callback\" (really, invoke JS code from C++ code) 2 ways:\r\n\r\n* calling the callback directly; if you have a `Handle<Function> foo` you can just `foo->Call()`\r\n* calling the callback via `MakeCallback` (in src/node.cc)\r\n\r\nThe former (\"direct\") case just calls from C++ into JS synchronously, and returns back into C++ when the JS code returns.\r\n\r\nThe latter (`MakeCallback`) case calls the JS callback function, but then also ends the current tick, calling `process._tickCallback()` and any *other* callbacks registered via `nextTick()`. This form is intended only for use by C++ code that was dispatched from the libuv event loop, and not for C++ code that was invoked from JS code. Because in that case (JS code \"A\" running, calls into C++ code in a node addon, the addon's C++ code calls MakeCallback, MakeCallback dispatches the nextTick callbacks) the JS code marked \"A\" has been preempted, and state changed by the nextTick callbacks can be corrupted. See https://github.com/TooTallNate/node-weak/issues/35 for an example of how dangerous this is.\r\n\r\nUse cases for node addons (that would want to use `nan`) wanting to invoke JS code from C++ code that was invoked directly from JS (not from a libuv event callback) include\r\n\r\n* node-weak (again, https://github.com/TooTallNate/node-weak/issues/35 is a practical example of this)\r\n* if you want to use V8 interceptors to provide Proxy-like abilities for a dynamic object to trap accesses and call JS handler routines\r\n\r\nFor these cases, use of `MakeCallback` is not only incorrect but extremely dangerous. The right answer is to call the callback directly. But `nan` doesn't expose a way of doing this; `NanCallback::Call` is actually wrapping `MakeCallback`. So is the right answer to avoid `NanCallback` altogether? I don't think so; it serves a very real need to deal more easily with V8's memory management; using `Persistent<Function>` got harder in newer V8 (node 0.11/0.12) and so it's much nicer to use `NanCallback` instead of raw `Handle<Function>`. So I think `NanCallback` should provide separate methods for calling a callback directly and calling a callback via `MakeCallback`, and make the difference really obvious so people don't accidentally do the latter. (Note in my [patch](https://github.com/TooTallNate/node-weak/pull/36) for the [node-weak instance of this](https://github.com/TooTallNate/node-weak/issues/35), I do use `NanCallback` for memory management, but then have to go to some lengths to extract the real `Handle<Function>` to call directly.)\r\n\r\nI think the problem here is exacerbated by (a) nebulous terminology like \"callback\" (is any function that gets called a \"callback\"? Not all callbacks are the same) and (b) the difficulty of using `Persistent<Function>` without `NanCallback`. But when you do use `NanCallback` and because of (a), it's just too tempting to accidentally call NanCallback::Call and thus MakeCallback and then preempt your JS code and then suffer all manner of heisenbugs.\r\n\r\nSo my suggestion and request is:\r\n\r\n* add a new `NanCallback::CallDirect` that wraps `Handle<Function>::Call`\r\n* rename `NanCallback::Call` to `NanCallback::CallAndEndTick` (or something similarly explicit that's difficult to confuse)\r\n* in a perfect world, remove `NanCallback::Call`. That will probably be too annoying to addon authors already using it correctly. Then, at least deprecate it (have it generate a compile-time warning, and then act as CallAndEndTick?)\r\n"},"comment":{"url":"https://api.github.com/repos/nodejs/nan/issues/comments/157096988","html_url":"https://github.com/nodejs/nan/issues/284#issuecomment-157096988","issue_url":"https://api.github.com/repos/nodejs/nan/issues/284","id":157096988,"user":{"login":"LinusU","id":189580,"avatar_url":"https://avatars.githubusercontent.com/u/189580?v=3","gravatar_id":"","url":"https://api.github.com/users/LinusU","html_url":"https://github.com/LinusU","followers_url":"https://api.github.com/users/LinusU/followers","following_url":"https://api.github.com/users/LinusU/following{/other_user}","gists_url":"https://api.github.com/users/LinusU/gists{/gist_id}","starred_url":"https://api.github.com/users/LinusU/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/LinusU/subscriptions","organizations_url":"https://api.github.com/users/LinusU/orgs","repos_url":"https://api.github.com/users/LinusU/repos","events_url":"https://api.github.com/users/LinusU/events{/privacy}","received_events_url":"https://api.github.com/users/LinusU/received_events","type":"User","site_admin":false},"created_at":"2015-11-16T16:58:56Z","updated_at":"2015-11-16T16:58:56Z","body":"I got bit by this today as well, I would love to see `NanCallback::CallDirect` implemented. Would a pull request be accepted?\r\n\r\nI'm also in favor for renaming `Call` to something more explicit, but that's step 2. I think we should add CallDirect either way..."}}