{"action":"created","issue":{"url":"https://api.github.com/repos/nodejs/node/issues/4507","labels_url":"https://api.github.com/repos/nodejs/node/issues/4507/labels{/name}","comments_url":"https://api.github.com/repos/nodejs/node/issues/4507/comments","events_url":"https://api.github.com/repos/nodejs/node/issues/4507/events","html_url":"https://github.com/nodejs/node/pull/4507","id":124560068,"number":4507,"title":"src: fix MakeCallback error handling","user":{"login":"trevnorris","id":289174,"avatar_url":"https://avatars.githubusercontent.com/u/289174?v=3","gravatar_id":"","url":"https://api.github.com/users/trevnorris","html_url":"https://github.com/trevnorris","followers_url":"https://api.github.com/users/trevnorris/followers","following_url":"https://api.github.com/users/trevnorris/following{/other_user}","gists_url":"https://api.github.com/users/trevnorris/gists{/gist_id}","starred_url":"https://api.github.com/users/trevnorris/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/trevnorris/subscriptions","organizations_url":"https://api.github.com/users/trevnorris/orgs","repos_url":"https://api.github.com/users/trevnorris/repos","events_url":"https://api.github.com/users/trevnorris/events{/privacy}","received_events_url":"https://api.github.com/users/trevnorris/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/nodejs/node/labels/c++","name":"c++","color":"e11d21"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":20,"created_at":"2016-01-01T20:21:49Z","updated_at":"2016-01-13T19:59:49Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/nodejs/node/pulls/4507","html_url":"https://github.com/nodejs/node/pull/4507","diff_url":"https://github.com/nodejs/node/pull/4507.diff","patch_url":"https://github.com/nodejs/node/pull/4507.patch"},"body":"The final goal of this PR is to allow `MakeCallback` (in both `node::` and `AsyncWrap::`) to be reentrant. Reason is because the `HTTPParser` may enter the JS call stack initially, or be called while already in a stack (see @indutny's PR https://github.com/nodejs/node/pull/4509). Also so native addon developers can always use `node::MakeCallback` without the worry of code being executed in the wrong order (specifically in relation to usage of `nextTick` or the `MicrotaskQueue`).\r\n\r\nTo do this I first made sure the two implementations of `MakeCallback` were the same. Previously they would return at different points in the case of caught exception.\r\n\r\nFinally was the addition of `AsyncCallbackScope` (admittedly the concept was taken from Fedor's PR above) to allow `MakeCallback` to be called recursively without each subsequent call from processing the `nextTickQueue` or the `MicrotaskQueue`.\r\n\r\nI am working with @misterdjules on the proper way of handling domains. So this PR is currently a WIP.\r\n\r\nR=@bnoordhuis \r\nR=@indutny \r\n\r\nIncluded is a commit by Fedor, who so graciously gave me a patch which was causing crypto tests to fail."},"comment":{"url":"https://api.github.com/repos/nodejs/node/issues/comments/171415684","html_url":"https://github.com/nodejs/node/pull/4507#issuecomment-171415684","issue_url":"https://api.github.com/repos/nodejs/node/issues/4507","id":171415684,"user":{"login":"misterdjules","id":811047,"avatar_url":"https://avatars.githubusercontent.com/u/811047?v=3","gravatar_id":"","url":"https://api.github.com/users/misterdjules","html_url":"https://github.com/misterdjules","followers_url":"https://api.github.com/users/misterdjules/followers","following_url":"https://api.github.com/users/misterdjules/following{/other_user}","gists_url":"https://api.github.com/users/misterdjules/gists{/gist_id}","starred_url":"https://api.github.com/users/misterdjules/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/misterdjules/subscriptions","organizations_url":"https://api.github.com/users/misterdjules/orgs","repos_url":"https://api.github.com/users/misterdjules/repos","events_url":"https://api.github.com/users/misterdjules/events{/privacy}","received_events_url":"https://api.github.com/users/misterdjules/received_events","type":"User","site_admin":false},"created_at":"2016-01-13T19:59:49Z","updated_at":"2016-01-13T19:59:49Z","body":"@trevnorris What do you think of doing the following in `node::*::MakeCallback`:\r\n\r\n1. Setting verbose = false on the `TryCatch` instance before the call to `Function::Call`.\r\n2. Calling `Function::Call`.\r\n3. Setting verbose = true on the `TryCatch` instance after the call to `Function::Call`.\r\n4. Call `TryCatch::Rethrow` if `TryCatch::HasCaught` returns true.\r\n\r\n? Setting the `TryCatch` instance to be non-verbose would allow us to perform the bookkeeping tasks that need to be done after the call to `Function::Call` without having subsequent calls to `Function::Call` (like when calling async wrap post hooks JS functions) always return the \"Empty\" value.\r\n\r\nRethrowing after setting verbose = true would make the error message to be sent so that it can be processed by `node::FatalException`, and thus any domain error handling or other error handling such as `process.on('uncaughtException')` could be done. It would also unwind the stack completely so that the behavior of throwing uncaught errors from JS functions called from `node::*::MakeCallback` would be consistent with the behavior of doing the same in other JS functions.\r\n\r\nThe question would now be: what to do if the async-wrap post-hook threw? What if both `Function::Call` threw and the async-wrap post-hook threw? Should both errors be re-thrown? In what order? But that’s more an async-wrap specific question, maybe it doesn’t need to be dealt with when making `node::*::MakeCallback` reentrant."}}