Skip to content

Commit

Permalink
src: check HasCaught() in JSStream calls
Browse files Browse the repository at this point in the history
`MakeCallback` can return an empty `MaybeLocal<>` even if no
exception has been generated, in particular, if
we were already terminating the current thread *before* the `TryCatch`
scope started, which meant it would not have an exception to report.
  • Loading branch information
addaleax committed Feb 15, 2019
1 parent 7d60b6b commit 0822101
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 5 deletions.
10 changes: 5 additions & 5 deletions src/js_stream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ bool JSStream::IsClosing() {
TryCatchScope try_catch(env());
Local<Value> value;
if (!MakeCallback(env()->isclosing_string(), 0, nullptr).ToLocal(&value)) {
if (!try_catch.HasTerminated())
if (try_catch.HasCaught() && !try_catch.HasTerminated())
FatalException(env()->isolate(), try_catch);
return true;
}
Expand All @@ -62,7 +62,7 @@ int JSStream::ReadStart() {
int value_int = UV_EPROTO;
if (!MakeCallback(env()->onreadstart_string(), 0, nullptr).ToLocal(&value) ||
!value->Int32Value(env()->context()).To(&value_int)) {
if (!try_catch.HasTerminated())
if (try_catch.HasCaught() && !try_catch.HasTerminated())
FatalException(env()->isolate(), try_catch);
}
return value_int;
Expand All @@ -77,7 +77,7 @@ int JSStream::ReadStop() {
int value_int = UV_EPROTO;
if (!MakeCallback(env()->onreadstop_string(), 0, nullptr).ToLocal(&value) ||
!value->Int32Value(env()->context()).To(&value_int)) {
if (!try_catch.HasTerminated())
if (try_catch.HasCaught() && !try_catch.HasTerminated())
FatalException(env()->isolate(), try_catch);
}
return value_int;
Expand All @@ -99,7 +99,7 @@ int JSStream::DoShutdown(ShutdownWrap* req_wrap) {
arraysize(argv),
argv).ToLocal(&value) ||
!value->Int32Value(env()->context()).To(&value_int)) {
if (!try_catch.HasTerminated())
if (try_catch.HasCaught() && !try_catch.HasTerminated())
FatalException(env()->isolate(), try_catch);
}
return value_int;
Expand Down Expand Up @@ -134,7 +134,7 @@ int JSStream::DoWrite(WriteWrap* w,
arraysize(argv),
argv).ToLocal(&value) ||
!value->Int32Value(env()->context()).To(&value_int)) {
if (!try_catch.HasTerminated())
if (try_catch.HasCaught() && !try_catch.HasTerminated())
FatalException(env()->isolate(), try_catch);
}
return value_int;
Expand Down
39 changes: 39 additions & 0 deletions test/parallel/test-worker-http2-generic-streams-terminate.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict';
const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

const assert = require('assert');
const http2 = require('http2');
const { Duplex } = require('stream');
const { Worker, isMainThread, workerData } = require('worker_threads');

// Tests the interaction between terminating a Worker thread and running
// the native SetImmediate queue, which may attempt to perform multiple
// calls into JS even though one already terminates the Worker.

if (isMainThread) {
const counter = new Int32Array(new SharedArrayBuffer(4));
const worker = new Worker(__filename, { workerData: { counter } });
worker.on('exit', common.mustCall(() => {
assert.strictEqual(counter[0], 1);
}));
} else {
const { counter } = workerData;

// Start two HTTP/2 connections. This will trigger write()s call from inside
// the SetImmediate queue.
for (let i = 0; i < 2; i++) {
http2.connect('http://localhost', {
createConnection() {
return new Duplex({
write(chunk, enc, cb) {
Atomics.add(counter, 0, 1);
process.exit();
},
read() { }
});
}
});
}
}

0 comments on commit 0822101

Please sign in to comment.