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

src: yield empty maybes for failed AsyncWrap::MakeCallback calls #22078

Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Aug 1, 2018

Use an empty MaybeLocal<Value> as the return value for
AsyncWrap::MakeCallback() if an exception occurred,
regardless of MakeCallback depth.

Unfortunately, the public API can not make this switch without
a breaking change (and possibly some kind of deprecation/renaming).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Use an empty `MaybeLocal<Value>` as the return value for
`AsyncWrap::MakeCallback()` if an exception occurred,
regardless of `MakeCallback` depth.

Unfortunately, the public API can not make this switch without
a breaking change (and possibly some kind of deprecation/renaming).
@addaleax addaleax requested a review from ryzokuken August 1, 2018 23:10
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 1, 2018
@addaleax
Copy link
Member Author

addaleax commented Aug 2, 2018

@addaleax
Copy link
Member Author

addaleax commented Aug 9, 2018

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 9, 2018
@jasnell
Copy link
Member

jasnell commented Aug 12, 2018

jasnell pushed a commit that referenced this pull request Aug 12, 2018
Use an empty `MaybeLocal<Value>` as the return value for
`AsyncWrap::MakeCallback()` if an exception occurred,
regardless of `MakeCallback` depth.

Unfortunately, the public API can not make this switch without
a breaking change (and possibly some kind of deprecation/renaming).

PR-URL: #22078
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
@jasnell
Copy link
Member

jasnell commented Aug 12, 2018

Landed in 45d0b4a

@rvagg
Copy link
Member

rvagg commented Aug 13, 2018

@addaleax would you mind backporting this to v10.x-staging please? There's a change in master that removed an additional IsInnerMakeCallback() that's not in v10.x and I'm not confident enough in what should be done about it so I'll leave it to you.

@addaleax addaleax deleted the async-wrap-makecallback-empty branch August 19, 2018 19:26
@addaleax
Copy link
Member Author

I think this applies cleanly now?

@targos
Copy link
Member

targos commented Aug 19, 2018

@addaleax it applies cleanly but fails to compile.

@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 19, 2018
addaleax added a commit to addaleax/node that referenced this pull request Aug 24, 2018
Use an empty `MaybeLocal<Value>` as the return value for
`AsyncWrap::MakeCallback()` if an exception occurred,
regardless of `MakeCallback` depth.

Unfortunately, the public API can not make this switch without
a breaking change (and possibly some kind of deprecation/renaming).

PR-URL: nodejs#22078
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
addaleax added a commit that referenced this pull request Aug 28, 2018
Use an empty `MaybeLocal<Value>` as the return value for
`AsyncWrap::MakeCallback()` if an exception occurred,
regardless of `MakeCallback` depth.

Unfortunately, the public API can not make this switch without
a breaking change (and possibly some kind of deprecation/renaming).

PR-URL: #22078
Backport-PR-URL: #22505
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
Use an empty `MaybeLocal<Value>` as the return value for
`AsyncWrap::MakeCallback()` if an exception occurred,
regardless of `MakeCallback` depth.

Unfortunately, the public API can not make this switch without
a breaking change (and possibly some kind of deprecation/renaming).

PR-URL: #22078
Backport-PR-URL: #22505
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2018
Use an empty `MaybeLocal<Value>` as the return value for
`AsyncWrap::MakeCallback()` if an exception occurred,
regardless of `MakeCallback` depth.

Unfortunately, the public API can not make this switch without
a breaking change (and possibly some kind of deprecation/renaming).

PR-URL: #22078
Backport-PR-URL: #22505
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants