audreyt / node-webworker-threads

Lightweight Web Worker API implementation with native threads
https://npmjs.org/package/webworker-threads
Other
2.3k stars 149 forks source link

HandleScope::HandleScope Entering the V8 API without proper locking in place #118

Closed legraphista closed 7 years ago

legraphista commented 8 years ago

Hi, Working with the following code:

const Threads = require('webworker-threads');

const threadPool = Threads.createPool(8);

function test (x){
    let xx = 0;
    for(let i = 0; i < 100000000; i++){
        xx = i;
    }
    console.log(`test ${x} ${xx}`);
}

threadPool.all.eval(test);

for(var i = 0; i < 100; i ++){
    threadPool.any.eval(`test(${i})`, (err) => console.error('callback', err.stack || err));
}

I can consistently reproduce the following error:

FATAL ERROR: HandleScope::HandleScope Entering the V8 API without proper locking in place
 1: node::Abort() [/Users/Stefan/.nvm/versions/node/v6.3.1/bin/node]
 2: node::FatalException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>) [/Users/Stefan/.nvm/versions/node/v6.3.1/bin/node]
 3: v8::HandleScope::Initialize(v8::Isolate*) [/Users/Stefan/.nvm/versions/node/v6.3.1/bin/node]
 4: node::FatalException(v8::Isolate*, v8::TryCatch const&) [/Users/Stefan/.nvm/versions/node/v6.3.1/bin/node]
 5: Callback(uv_async_s*, int) [/Users/Stefan/Desktop/Work/node-playground/node_modules/webworker-threads/build/Release/WebWorkerThreads.node]
 6: uv__async_event [/Users/Stefan/.nvm/versions/node/v6.3.1/bin/node]
 7: uv__async_io [/Users/Stefan/.nvm/versions/node/v6.3.1/bin/node]
 8: uv__io_poll [/Users/Stefan/.nvm/versions/node/v6.3.1/bin/node]
 9: uv_run [/Users/Stefan/.nvm/versions/node/v6.3.1/bin/node]
10: node::Start(int, char**) [/Users/Stefan/.nvm/versions/node/v6.3.1/bin/node]
11: start [/Users/Stefan/.nvm/versions/node/v6.3.1/bin/node]
12: 0x4

Node: 4.5.0, 6.3.1, 6.5.0 OS: OSX 10.11.6

EDIT 1: The issue was caused due to an error in the callback,err is null (silly me), so it would throw when accessing err.stack I think this is still an issue because the error in the callback did not bubble up and did not appear, even with process.on('uncaughtException', (err)=>console.error(err)) present.

brodycj commented 8 years ago

Similar problem demonstrated by the following test case in test-package.js:

tap.test('eval pool with callback that throws', function(t) {
  var tcount = 0;
  function test() {
    console.log('test function in thread pool');
  }

  var mypool = Threads.createPool(5);
  //mypool.all.eval(test);
  mypool.all.eval('test()', function(err, data) {
    ++tcount;
    if (tcount === 2)
      throw new Error('boom');
    if (tcount == 5) {
      mypool.destroy();
      t.end();
    }
  });
});
brodycj commented 8 years ago

The following change (with JavaScript code regenerated by npm run js) seems to fix the problem with the original code but not in the test case above:

diff --git a/src/createPool.ls b/src/createPool.ls
index f12894b..f464560 100644
--- a/src/createPool.ls
+++ b/src/createPool.ls
@@ -41,7 +41,10 @@ function create-pool (n)
                     next-job t
                     f = job.cb-or-data
                     if typeof f is \function
-                      f.call t, e, d
+                      try
+                        f.call t, e, d
+                      catch e
+                        return e
                     else
                       t.emit job.src-text-or-event-type, f
             else if job.type is 2 # EMIT

Going to sleep now, hope to give it a try when I am up again.

creole commented 7 years ago

Any progress on this? Running into the exact same error. Node: 6.9.2 OS: Debian GNU/Linux 8 (jessie)

audreyt commented 7 years ago

Fixed in webworker-threads-0.7.10. Thanks for the reminder!

creole commented 7 years ago

Thank you, will test later.

davisjam commented 7 years ago

@audreyt I must be missing something here. What does 0d9a167 actually accomplish? I see it must be important, since you added it again in 1a3fbed2 after I removed it in #141. But with RAII, what is the effect of creating a Locker within the scope of an otherwise empty if statement?

davisjam commented 7 years ago

@brodybits I don't see that test case in test-package.js. Since I appear to have inadvertently re-introduced this bug until @audreyt fixed it in 1a3fbed, perhaps adding it to the test suite would be helpful?