Unhandled exceptions and infinite loop on mac

[bjouhier]

I'm running into a strange bug on mac osx: exceptions are not caught properly in the C++ code. Instead I'm getting a terminate called after throwing an instance of 'NodeOracleException' message and the node process goes into an infinite loop (and it takes a kill -9 to kill it).

This occurs in several places. One example is with following stack:

ExecuteBaton::CopyValuesToBaton
ExecuteBaton::ExecuteBaton
Connection::Execute

If execution reaches the throw at then end of CopyValuesToBaton because val is undefined then the exception should be caught by the following block in Connection::Execute:

try {
    baton = new ExecuteBaton(connection, *sqlVal, &values, NULL);
  } catch(NodeOracleException &ex) {
    return ThrowException(Exception::Error(String::New(ex.getMessage().c_str())));
  }

But it is not!!

I'm suspecting either a problem with compiler options or a memory corruption.

I managed to get rid of the infinite loop by recompiling the driver without the -Os option: the exception is still not caught but I'm getting an abort trap 6 instead. But I did not manage to fix the problem by playing with the other options. I tried to remove the -fno-rtti and add a -fexceptions but had no luck.

I'm running node 0.10.20 on OSX 10.8.5

[bjouhier]

Another note: memory corruption seems a bit unlikely because the driver runs great when there is no exception.

[johannish]

Could you paste a section of the corresponding Javascript code which causes this exception?

I had the same C++ error (terminate called after throwing an instance...) related to #74. In that case the problem was that the destructor itself threw an exception, leaving the object in memory. When the V8 garbage collector came around some time later and tried to call the destructor again, it would throw terminate called after throwing..., and the Node process would crash. However, your problem sounds different.

I also notice that the section of code you quoted from Connection::Execute is not current. Are you sure you have the latest code? See: https://github.com/nearinfinity/node-oracle/blob/master/src/connection.cpp#L61-L65

[bjouhier]

Thanks a lot for the info. I'll update my fork and investigate further, with special attention to the destructor.

[bjouhier]

I upgraded to the latest code but it did not make any difference.

I tried to disable all the destructors but it did not make any difference either.

Here is my test code:

var oracle = require('oracle');

console.log("connecting ...");
oracle.connect(require('./testConfig.json'), function(err, cnx) {
    if (err) throw err;
    console.log("connected");
    cnx.execute("create table FOO (BAR varchar(10))", [], function(err) {
        if (err && !/^ORA-00955/.test(err.message)) throw err;
        console.log("table created");
        cnx.execute("insert into FOO (BAR) values (:1)", [undefined], function(err) {
            if (err) throw err;
            console.log("row inserted");
        });
    });
})

Note: code works if I replace undefined by "ZOO"

[bjouhier]

Output is:

$ node ../../oracle/bug
connecting ...
connected
table created
terminate called after throwing an instance of 'NodeOracleException'
<hanging here and taking 100% of one CPU core>

[bjouhier]

I experimented a bit more. I trimmed ExecuteBaton::CopyValuesToBaton.

The exception is properly caught if I trim it down to:

void ExecuteBaton::CopyValuesToBaton(ExecuteBaton* baton, v8::Local<v8::Array>* values) {
  uint32_t len = (*values)->Length();
  throw NodeOracleException("test");
  for(uint32_t i=0; i<len; i++) {
  }
}

But I get the problem as soon as I move the throw inside the loop:

void ExecuteBaton::CopyValuesToBaton(ExecuteBaton* baton, v8::Local<v8::Array>* values) {
  uint32_t len = (*values)->Length();
  for(uint32_t i=0; i<len; i++) {
    throw NodeOracleException("test");
  }
}

It does not make much sense to me but that's how it is!

[bjouhier]

I've managed to reproduce the problem with a simple addon which is independent from node-oracle and not linked to instantclient: https://gist.github.com/bjouhier/7307287

So this is not an oracle specific bug, rather an issue with addons on osx. I'm posting it as a node issue.

[bjouhier]

@raztus @joeferner

Node and V8 are compiled with -fno-exceptions. So add-ons should never mix v8/node AP calls and exception handling.

This means that instantclient calls should be wrapped with try/catch but exceptions should not be used to transfer control to the rest of the driver code.

I can restructure the driver code to fix this and submit a PR but I'd like to have your opinion before.

[johannish]

@raymondfeng Might have better input than I do. I'm quite inexperienced with C++.

Could you provide an example of where we are mixing v8/node calls and our own exceptions, and would you help me understand what the issue is?

If I'm understanding correctly, you're talking about code like these lines from Connection::Execute:

  } catch(NodeOracleException &ex) {
    return scope.Close(ThrowException(Exception::Error(String::New(ex.getMessage().c_str()))));
  }

But if that is an example of what you're referring to, I fail to understand the problem, since that looks acceptable to me.

[bjouhier]

@raztus @raymondfeng

Problem is with https://github.com/nearinfinity/node-oracle/blob/master/src/executeBaton.cpp#L133. The ExecuteBaton::CopyValuesToBaton method contains calls to V8 APIs (v8::Local for example) and throws an exception which is caught in Connection::Execute. Behavior is unpredictable because exceptions are being used in conjunction with code that was not compiled for EH. If you are lucky it may work but it may also fail badly, as I experienced on OSX.

See comments in nodejs/node-v0.x-archive#6463

[raymondfeng]

I'm not sure if that's true. First, the node-oracle addon is compiled and linked with exception handling enabled. Second, the code throwing the exception (https://github.com/nearinfinity/node-oracle/blob/master/src/executeBaton.cpp#L133) and catching the exception (https://github.com/nearinfinity/node-oracle/blob/master/src/connection.cpp#L60) are both from the addon itself.

[raymondfeng]

I'll look into https://gist.github.com/bjouhier/7307287 more.

[bjouhier]

Yes, the throw and catch are both in the add-on but we don't know the assumptions that the compiler made when it compiled constructors and destructors for v8::Local, for example. These assumptions might be incompatible with code that throws an exception though a block containing a v8::Local.declaration. At least it looks like this is what I ran into.

Note that my gist fails with gcc 4.7 but works ok with gcc 4.2. The gist compiles with both but node-oracle only compiles with 4.7 - I get compile errors on ostringstream with 4.2.