Correct abstraction for try/catch

[mhdawson]

• Version: ALL
• Platform: ALL
• Subsystem: API

Comment from Ben in : nodejs/node-eps#20 (comment)

I think most JS engines have an 'is exception pending?' function; V8 is the outlier with its TryCatch construct. API strawman:

// Wrapper around a v8::TryCatch. Dummy with other engines.
typedef struct napi_trycatch { struct napi_trycatch *v; } napi_trycatch;
NODE_EXTERN void napi_trycatch_new(napi_env e, napi_trycatch *trycatch);
// Returns exception object or undefined.
NODE_EXTERN napi_value napi_trycatch_exception(napi_env e, napi_trycatch trycatch);
NODE_EXTERN void napi_trycatch_delete(napi_env e, napi_trycatch trycatch);

[gabrielschulhof]

The V8 documentation claims that the TryCatch should be stack-allocated, so I'm not sure if we can go with the new/delete paradigm. If we can't, we may have to provide a way of calling a function that also catches exceptions, like maybe with an out parameter:

napi_value
napi_call_function_with_exception(napi_env e,
                                  napi_value scope,
                                  napi_value func,
                                  int argc,
                                  napi_value* argv,
                                  napi_value* exception);

Then, applications can simply re-throw the exception if our implementation sets it.

OTOH, if we can heap-allocate, IINM in the V8 implementation we have to remember to v8::TryCatch::ReThrow() the exception in napi_trycatch_delete() if napi_trycatch_exception() has not been called because V8 automatically removes the exception state if a TryCatch is within scope. So, in V8 we need at least one additional piece of data in addition to the v8::TryCatch itself: A sticky bit exception_retrieved which will be set in napi_trycatch_exception() to indicate that we need not call ReThrow() in napi_trycatch_delete(), because the exception has been retrieved.

This way, we have the JsGetAndClearException() semantics in napi_trycatch_exception(), but also the need to have an object that bookends the transaction that may throw à la v8::TryCatch.

Unfortunately this means that, if somebody leaks the napi_trycatch by failing to call napi_trycatch_delete() then V8 and ChakraCore will behave differently because, in V8, the exception will have been caught, whereas in ChakraCore it will have been left in place.

[aruneshchandra]

@fhinkel - can you comment on this ?

[gabrielschulhof]

During our last meeting, I learned that there are other V8 structures that are also documented as having to be stack-allocated yet work fine when heap-allocated. So, I went ahead and created an implementation of @mhdawson's API with a heap-allocated v8::TryCatch and the boolean flag as I described in my comment. I also created an implementation for ChakraCore.

[hashseed]

The reason v8::TryCatch ought to be stack-allocated is that nested v8::TryCatch scopes build a chain that reflect this nesting. Heap-allocating would be possible, but must make sure that the allocation/deallocation order is kept. Otherwise things might break in weird ways. Letting it escape via proposed API seems an easy source for bugs.

[fhinkel]

@hashseed do you think the API could abstract it in such a way that it can't be used incorrectly?

[hashseed]

Well you could, maybe in debug mode, track the nesting inside napi_trycatch_* and make sure we can only call napi_trycatch_delete on the last v8::TryCatch in the chain. But if the user would use v8::TryCatch intermixed with napi_trycatch_*, we could still deallocate out of order.

[gabrielschulhof]

What about implementing the ChakraCore abstraction instead? We could surround all calls into JS (napi_call_function()) with a v8::TryCatch, and store a possible exception in a Persistent. Then, that would be the "last known exception", and a call to napi_trycatch_exception() would return it and clear the persistent.

Otherwise, if nobody calls napi_trycatch_exception(), napi_trycatch_delete() would throw the exception present in the persistent, if any.

Is there a big performance penalty when surrounding a call into JS with a v8::TryCatch?

[hashseed]

I think surrounding all calls into V8 with a v8::TryCatch is a very good idea. TryCatch scopes are not particularly expensive to allocate. If no exception is thrown, it should have almost no performance impact. If the exception is thrown and you need to rethrow, then you add some churn there.

[gabrielschulhof]

api-prototype-6.2.0...gabrielschulhof:try-catch-6.2.0-global-persistent-exception shows the global persistent exception approach.

[aruneshchandra]

Error handling design has covered this issue - but we need to track the performance impact of this. Closing this issue for now - either reopen or file new issue if problem arises.