Execution in pending exception state #122
Comments
|
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 |
|
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 ? |
|
@fhinkel - question for you ? Should we be preventing calling the APIs when there are exception pending ? |
|
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. |
|
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>
.
|
|
@mhdawson @gabrielschulhof So did we decide on exactly what we're going to do here? Are we just going to remove the |
|
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 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.
|
|
The obvious ones that need to be unchecked are for closing handle scopes ( I'm not sure about others. Some candidates might be:
|
|
@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. |
|
@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. |
N-API is somewhat strict about blocking calls to many APIs while there is a pending exception. The NAPI_PREAMBLE macro at the beginning of many API implementations checks for a pending exception. However, a subset of the APIs (which don't call back into JavaScript) still need to work while in a pending-exception state. This changes the reference APIs (equivalent to v8::Persistent) and handle scope APIs so that they can be used for cleanup up while an exception is pending. We may decide to similarly enable a few other APIs later, (which would be a non-breaking change) but we know at least these are needed now to unblock some specific scenarios. Fixes: nodejs/abi-stable-node#122
|
@jasongin Could this be reflected in documentation at https://nodejs.org/api/n-api.html? What I mean is, it would be useful both for users and implementors to have explicitly stated which methods would be still executed and which would immediately return if exception is already pending. |
N-API is somewhat strict about blocking calls to many APIs while there is a pending exception. The NAPI_PREAMBLE macro at the beginning of many API implementations checks for a pending exception. However, a subset of the APIs (which don't call back into JavaScript) still need to work while in a pending-exception state. This changes the reference APIs (equivalent to v8::Persistent) and handle scope APIs so that they can be used for cleanup up while an exception is pending. We may decide to similarly enable a few other APIs later, (which would be a non-breaking change) but we know at least these are needed now to unblock some specific scenarios. Fixes: nodejs/abi-stable-node#122 Fixes: nodejs/abi-stable-node#228 PR-URL: nodejs#12524 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
N-API is somewhat strict about blocking calls to many APIs while there is a pending exception. The NAPI_PREAMBLE macro at the beginning of many API implementations checks for a pending exception. However, a subset of the APIs (which don't call back into JavaScript) still need to work while in a pending-exception state. This changes the reference APIs (equivalent to v8::Persistent) and handle scope APIs so that they can be used for cleanup up while an exception is pending. We may decide to similarly enable a few other APIs later, (which would be a non-breaking change) but we know at least these are needed now to unblock some specific scenarios. Fixes: nodejs/abi-stable-node#122 Fixes: nodejs/abi-stable-node#228 Backport-PR-URL: #19447 PR-URL: #12524 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
NAPI currently has a policy that any API call during a pending-exeption state will bail out immediately with a
napi_pending_exceptionerror. 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-sassmodule has pretty good unit test coverage of error conditions, and a few of those error tests are hitting this problem.The text was updated successfully, but these errors were encountered: