terminate execution requires isExecutionTerminating() checks in C++ callbacks

[GoogleCodeExporter]

I have been experimenting with killing V8 using v8::V8::TerminateExecution 
specifically calling it from python using:

def jsKillThread():
    print "terminating v8 script execution"
    PyV8.JSEngine().terminateAllThreads()

killThread = threading.Timer(killTimeout, jsKillThread)

This obviously calls the underlying V8 TerminateExecution:

void CEngine::TerminateAllThreads(void) {
    v8::V8::TerminateExecution();
}

So what happens when that is called is that V8 will create an exception that 
can not be caught inside of JavaScript, and so the javascript stack unwinds all 
the way and the script->Run() call finishes.   

If the script is inside of a C++ callback we need to return immediately.   

From the TryCatch::CanContinue() documentation:

"Currently, the only type of exception that can be caught by a TryCatch handler 
and for which it does not make sense to continue is termination exception. Such 
exceptions are thrown when the TerminateExecution methods are called to 
terminate a long-running script.

If CanContinue returns false, the correct action is to perform any C++ cleanup 
needed and then return."

So we know that the TerminateExecution call can only be called when a thread 
has control.  It makes sense to check if TerminateExecution is being called in 
the C++ callback methods after the GIL is reaquired and then bail out so we 
don't allocate any V8 objects while it is trying to terminate (this causes 
segfaults if empty handles get accessed).  We can check if it has been called 
using v8::V8::IsExecutionTerminating().

I added a macro to Wrapper.cpp as follows:

#define TERMINATE_EXECUTION_CHECK(returnValue) \
    if(v8::V8::IsExecutionTerminating()) { \
        PyErr_Clear(); \
        PyErr_SetString(PyExc_RuntimeError, "execution is terminating"); \
        return returnValue; \
    }

I then added the macro after each CPythonGIL python_gil; call:

CPythonGIL python_gil;
TERMINATE_EXECUTION_CHECK(v8::Handle<v8::Integer>())  

You have to change the returnValue to whatever the C++ method is expecting.  

In Exception.cpp CJavascriptException::ThrowIf I added the extra check for 
CanContinue():

if ( try_catch.HasCaught() && try_catch.CanContinue() ){

Finally in Engine.cpp CEngine::ExecuteScript I added the extra throw: 

Py_BEGIN_ALLOW_THREADS
result = script->Run();
Py_END_ALLOW_THREADS

if(result.IsEmpty()) {
    if (try_catch.HasCaught()) {
        if( !try_catch.CanContinue() && PyErr_Occurred() ) {
            throw py::error_already_set();         
        }

My testing has shown that this is fairly stable and working well.  This is not 
really a bug because I expect not many people are playing with 
TerminateExecution, but if they are... it could segfault in weird places.  Im 
not that experienced with C++ or V8 or Boost-Python or the Python-C API so let 
me know if anything looks really wrong.  

Original issue reported on code.google.com by ATM1...@gmail.com on 18 Aug 2011 at 11:30

[GoogleCodeExporter]

Great, I agree that is a design issue, because I haven't put too many effort on 
the multithread. I have checked in your patch, and I will double check the 
remaining CPythonGIL later. Please verify it with SVN trunk after r394

Thanks very much!

Original comment by flier...@gmail.com on 19 Aug 2011 at 7:47

• Changed state: Fixed
• Added labels: OpSys-All

[GoogleCodeExporter]

Thanks, I will be doing some more tests with it to make sure all of those 
checks are really needed and also to make sure a check is not missed.  V8 has 
very few documentation about what the state of the VM is in when the execution 
is terminating.  Its been basically lots of experimentation with standalone C++ 
programs to see that segfaults could occur due to access on empty handles.  
Sometimes you get a fatal error as they have used assert to check inside of V8. 

Original comment by ATM1...@gmail.com on 19 Aug 2011 at 3:55

[GoogleCodeExporter]

Thanks, I will also monitor the crash, maybe you could use a debug build which 
could generate the core file, and we could dump the stack trace.

Original comment by flier...@gmail.com on 19 Aug 2011 at 3:57