nodejs / abi-stable-node

Repository used by the Node-API team to manage work related to Node-API and node-addon-api
239 stars 47 forks source link

napi_create_int32 does not return napi_pending_exception #356

Closed GiedriusM closed 5 years ago

GiedriusM commented 5 years ago

I noticed that napi_create_int32 (and other "Functions to convert from C types to N-API") do not return napi_pending_exception when one is actually present, however napi_create_buffer (and other "Object Creation Functions") do.

Is this inconsistency intended? According to this old post it should not (napi_create_number is checked as well as napi_create_buffer). Could not find any newer information, though.

Got really confused when the code started failing in the middle or variable creation :/

mhdawson commented 5 years ago

I believe it started as an effort to allow you to call functions which would be needed to do minimal cleanup in the face of a pending exception. This may have been extended to cases were the methods cannot throw an exception themselves.

Do you have some sample code that shows where the exception was generated but then only later you have it returned?

GiedriusM commented 5 years ago

Just to be clear, I do not consider this as a bug, but more of inconsistent behavior, which was weird to debug. I encountered this when doing node.js wrappers for a 3rd party C library. The basic call stack was like this:

node_main -> 
  wrap_process() -> 
    lib_process ->
      // if certain condition is met, call:
      wrap_callback_A() ->
        napi_call_function ->
          node_callback_A (which throws)

      // normally at this stage we should return to Node.js context, but
      // library does not know anything about pending exception
      // or node.js for that matter, and continues to execute

      // if some other condition is met, call:
      wrap_callback_B() ->
        napi_create_int32() // succeeds
        napi_create_buffer() // fails if wrap_callback_A was called and node_callback_A threw exception
        napi_call_function -> // not rea
          node_callback_B()

Technically, wrap_callback_B should check for pending exception and return early, but it didn't because, well, I did not expect it would be called with a pending exception. Instead I got invalid statuses somewhere in the middle of wrap_callback, when the napi_create_buffer failed.

mhdawson commented 5 years ago

Thanks for the detail. I do see why the first call passing, but the second one not could be confusing.

I assume napi_call_function would have returned that there was a pending exception? My guess is that would have been the right place to check and return, but I'm not quite sure from what you've provided above.

GiedriusM commented 5 years ago

Thanks for the detail. I do see why the first call passing, but the second one not could be confusing.

Exactly this.

I assume napi_call_function would have returned that there was a pending exception? My guess is that would have been the right place to check and return, but I'm not quite sure from what you've provided above.

Yes, napi_call_function (A), did return pending exception and wrap_callback_A() handled that (early return with error code). However, in my situation the 3rd party library despite/due-to the error, continues it's process loop and calls wrap_callback_B(), which could not complete due to the pending exception. To give another shot at describing, this would more or less be the node.js side code:


const wrap = require('wrapper');

const w = new wrap.wrap();

w.on('eventA', () => throw new Error());
w.on('eventB', () => console.log('B'));

// At some point eventA and eventB are called in the same process loop
setInterval(() => w.process(), 100);

Anyway, getting back to the point. Is this discrepancy between napi_create_X functions a feature or a bug?

mhdawson commented 5 years ago

It is a "feature" that we have chosen that you can call some API functions even though an exception is pending. This was to allow for minimal cleanup to be done if possible. I will take a look to see if we properly document that.

mhdawson commented 5 years ago

I'm going to update the docs to reflect the current behavior as they don't explain the case where one is already pending when you call a method.

I won't document the specific methods yet as I'm not sure we want to commit to which ones can/can not be called.

mhdawson commented 5 years ago

Created https://github.com/nodejs/node/pull/25339 to clarify behaviour in the docs

GiedriusM commented 5 years ago

Alright, thanks.

Also do you have any suggestion if there's a way for a C function to force Node.js handle pending exception, without returning? Sort of recursive node.js core loop handing? In my scenario I can't control when wrap_callback_B is called (without reworking the 3rd party lib) and if there's a already a pending exception it can't continue and is forced to skip and event. I guess it's more of an application architecture issue, but still.

mhdawson commented 5 years ago

One thing I can think of would be to check for a pending exception and then calling napi_fatal_error which will end up terminating the process. That is also the likely outcome of the pending exception percolating up as if you cannot resolve the exception on the native side then its unlikely the JavaScript code can either.

GiedriusM commented 5 years ago

Alright, thank you for the help. As far as I'm concerned, this issue can be closed.

mhdawson commented 5 years ago

@GiedriusM thanks for your help/discussion. Closing for now.