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.

Sign up for GitHub

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

Jump to bottom

noexcept control flow issues #419

Closed
DaAitch opened this issue Dec 28, 2018 · 0 comments
Closed

noexcept control flow issues #419

DaAitch opened this issue Dec 28, 2018 · 0 comments

Comments

@DaAitch
Copy link
Contributor

DaAitch commented Dec 28, 2018

I think there are two noexcept issues with control flow.

1. NAPI_THROW noexcept control flow

Like NAPI_THROW_IF_FAILED*, NAPI_THROW should also always abort control flow, but noexcept definition doesn't have a return so execution continues. All implementations using NAPI_THROW have a trailing return statement, so there is no bug, but I'd say it's unclean.

// for noexcept
#define NAPI_THROW(e)  (e).ThrowAsJavaScriptException();

2. noexcept ThrowAsJavaScriptException may produce infinite recursion

if napi_throw fails, it's likely that all further calls are failing and every time invoke another ThrowAsJavaScriptException, so I think this is a fatal case. Recursion is not a good idea, and also ignoring it, so it should fatal abort.
DaAitch added a commit to DaAitch/node-addon-api that referenced this issue Dec 28, 2018
@DaAitch
src: fix noexcept control flow issues
648dbd4 
- adapt `NAPI_THROW`, create `NAPI_THROW_VOID`: either throw or
  return, never continue
- fix `Error::ThrowAsJavaScriptException`: if `napi_throw` fails, either
  explicitly throw or create fatal error

Closes: nodejs#419
@DaAitch DaAitch mentioned this issue Dec 28, 2018
Closed
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this issue Aug 24, 2022
@kevindavies8
src: fix noexcept control flow issues
ce332e7 
- adapt `NAPI_THROW`, create `NAPI_THROW_VOID`: either throw or
  return, never continue
- fix `Error::ThrowAsJavaScriptException`: if `napi_throw` fails, either
  explicitly throw or create fatal error

Fixes: nodejs/node-addon-api#419
PR-URL: nodejs/node-addon-api#420
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this issue Aug 26, 2022
@Marlyfleitas
src: fix noexcept control flow issues
91fd27b 
- adapt `NAPI_THROW`, create `NAPI_THROW_VOID`: either throw or
  return, never continue
- fix `Error::ThrowAsJavaScriptException`: if `napi_throw` fails, either
  explicitly throw or create fatal error

Fixes: nodejs/node-addon-api#419
PR-URL: nodejs/node-addon-api#420
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this issue Sep 19, 2022
@wroy7860
src: fix noexcept control flow issues
ac869d3 
- adapt `NAPI_THROW`, create `NAPI_THROW_VOID`: either throw or
  return, never continue
- fix `Error::ThrowAsJavaScriptException`: if `napi_throw` fails, either
  explicitly throw or create fatal error

Fixes: nodejs/node-addon-api#419
PR-URL: nodejs/node-addon-api#420
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this issue Aug 11, 2023
src: fix noexcept control flow issues
e5301cc 
- adapt `NAPI_THROW`, create `NAPI_THROW_VOID`: either throw or
  return, never continue
- fix `Error::ThrowAsJavaScriptException`: if `napi_throw` fails, either
  explicitly throw or create fatal error

Fixes: nodejs/node-addon-api#419
PR-URL: nodejs/node-addon-api#420
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

src: fix noexcept control flow issues DaAitch/node-addon-api
1 participant
@DaAitch