Execution in pending exception state

[jasongin]

NAPI currently has a policy that any API call during a pending-exeption state will bail out immediately with a napi_pending_exception error. That policy is too restrictive.

It is often necessary for native modules to do some cleanup operations while in a pending-exception state. Most commonly they may need to close a handle scope. But also other things like releasing/deleting a reference should be allowed. I'm not sure about setting a property on an object (to an error state)... we should probably block anything that has a chance of re-entering JS code.

The node-sass module has pretty good unit test coverage of error conditions, and a few of those error tests are hitting this problem.

[gabrielschulhof]

Perhaps we should not take it upon ourselves to decide whether to forward the call to the VM or not. After all, a VM has to be able to deal with the case where there's a pending exception but a call into it has been made.

ChakraCore should simply return something other than JsNoError and V8 should have its own mechanisms for indicating an exceptional state, such as an empty maybe.

[mhdawson]

I'm leaning towards the suggestion to just allow the vm to handle the problem without us deciding to forward or not. Would it make sense to try that with node-sass to see if there are any problems that show up since it sounds like it has reasonable coverage of error conditions ?

[aruneshchandra]

@fhinkel - question for you ? Should we be preventing calling the APIs when there are exception pending ?

[fhinkel]

In V8, it's the embedder's responsibility to use a TryCatch or continue appropriately if there was an exception. If they ignore the exception and call V8 again, it'll usually result in buggy code.

[gabrielschulhof]

To me this sounds like it's an application-specific decision and thus beyond the scope of N-API. All we can do is faithfully convey the state of the VM.

…

On Fri, Mar 10, 2017 at 11:02 AM, F. Hinkelmann ***@***.***> wrote: In V8, it's the embedder's responsibility to use a TryCatch or continue appropriately if there was an exception. If they ignore the exception and call V8 again, it'll usually result in buggy code. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#122 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7k0d4nPfWgQmlaRnHfVa1TROxbFoJSks5rkRG-gaJpZM4MQq6Q> .

[jasongin]

@mhdawson @gabrielschulhof So did we decide on exactly what we're going to do here? Are we just going to remove the NAPI_PREAMBLE from selected functions where we know they need to work while there is a pending exception?

[gabrielschulhof]

The fact is that in V8 we are clearing any exception that has occurred because we are catching it and storing it in the last exception. Thus, we will not benefit from whatever internal logic V8 has for dealing with a pending exception. In turn, we must thus reproduce any failures it would have :/ This is the price we pay for imposing the ChakraCore exception handling approach onto V8.

To reproduce V8's behaviour we put NAPI_PREAMBLE() in those places where V8 would fail. Currently the exhaustive list is below (checked means "uses NAPI_PREAMBLE()").

For the purposes of discussion, let's accompany each uncheck with a comment as to why it can be unchecked. We can then implement the final list as a PR.

• napi_get_last_error_info
• napi_get_boolean
• napi_typeof
• napi_get_undefined
• napi_get_null
• napi_get_cb_info
• napi_get_cb_args_length
• napi_is_construct_call
• napi_get_cb_args
• napi_get_cb_this
• napi_get_cb_data
• napi_get_global
• napi_is_error
• napi_get_value_double
• napi_get_value_int32
• napi_get_value_uint32
• napi_get_value_int64
• napi_get_value_bool
• napi_unwrap
• napi_delete_reference
• napi_is_exception_pending
• napi_get_and_clear_last_exception
• napi_create_function
• napi_define_class
• napi_set_return_value
• napi_get_property_names
• napi_set_property
• napi_has_property
• napi_get_property
• napi_set_named_property
• napi_has_named_property
• napi_get_named_property
• napi_set_element
• napi_has_element
• napi_get_element
• napi_define_properties
• napi_is_array
• napi_get_array_length
• napi_strict_equals
• napi_get_prototype
• napi_create_object
• napi_create_array
• napi_create_array_with_length
• napi_create_string_utf8
• napi_create_string_utf16
• napi_create_number
• napi_create_symbol
• napi_create_error
• napi_create_type_error
• napi_create_range_error
• napi_call_function
• napi_throw
• napi_throw_error
• napi_throw_type_error
• napi_throw_range_error
• napi_get_value_string_length
• napi_get_value_string_utf8
• napi_get_value_string_utf16
• napi_coerce_to_object
• napi_coerce_to_bool
• napi_coerce_to_number
• napi_coerce_to_string
• napi_wrap
• napi_create_external
• napi_get_value_external
• napi_create_reference
• napi_reference_ref
• napi_reference_unref
• napi_get_reference_value
• napi_open_handle_scope
• napi_close_handle_scope
• napi_open_escapable_handle_scope
• napi_close_escapable_handle_scope
• napi_escape_handle
• napi_new_instance
• napi_instanceof
• napi_make_callback
• napi_create_buffer
• napi_create_external_buffer
• napi_create_buffer_copy
• napi_is_buffer
• napi_get_buffer_info
• napi_is_arraybuffer
• napi_create_arraybuffer
• napi_create_external_arraybuffer
• napi_get_arraybuffer_info
• napi_is_typedarray
• napi_create_typedarray
• napi_get_typedarray_info

[jasongin]

The obvious ones that need to be unchecked are for closing handle scopes (napi_close_handle_scope, napi_close_escapable_handle_scope), to ensure scopes are not left unclosed when bailing out due to an exception.

I'm not sure about others. Some candidates might be:

• Reference counting APIs (napi_reference_ref, napi_reference_unref) because you may need to unreference an object (or reference an error object?) if there was an exception.
• Other handle scope APIs (napi_escape_handle, napi_open_handle_scope, napi_open_escapable_handle_scope) just for consistency? Or maybe you need an Error object to escape from the scope?
• APIs that only read from a value, without any possibility of executing JS code: napi_is_error, napi_is_array, napi_is_arraybuffer, napi_is_typedarray, napi_is_buffer, napi_get_value_external, napi_get_value_string_*, napi_get_arraybuffer_info, napi_get_typedarray_info, napi_get_buffer_info, ... (maybe more?)

[jasongin]

@gabrielschulhof Are you working on this, or should I?

I think we should try to fix this before the v8.0 release if possible, since it is the only known blocking issue in the APIs.

[gabrielschulhof]

@jasongin I think you better take it, if you have the bandwidth, because I almost certainly don't have enough to do it before v8.0.