Skip to content

Commit dafa380

Browse files
committed
worker: emit 'messagerror' events for failed deserialization
This is much nicer than just treating exceptions as uncaught, and enables reporting of exceptions from the internal C++ deserialization machinery. PR-URL: #33772 Backport-PR-URL: #33965 Reviewed-By: Benjamin Gruenbaum <[email protected]>
1 parent dd51ba3 commit dafa380

9 files changed

+69
-6
lines changed

doc/api/errors.md

+12
Original file line numberDiff line numberDiff line change
@@ -1539,6 +1539,17 @@ behavior. See the documentation for [policy][] manifests for more information.
15391539
An attempt was made to allocate memory (usually in the C++ layer) but it
15401540
failed.
15411541

1542+
<a id="ERR_MESSAGE_TARGET_CONTEXT_UNAVAILABLE"></a>
1543+
### `ERR_MESSAGE_TARGET_CONTEXT_UNAVAILABLE`
1544+
<!-- YAML
1545+
added: REPLACEME
1546+
-->
1547+
1548+
A message posted to a [`MessagePort`][] could not be deserialized in the target
1549+
[vm][] `Context`. Not all Node.js objects can be successfully instantiated in
1550+
any context at this time, and attempting to transfer them using `postMessage()`
1551+
can fail on the receiving side in that case.
1552+
15421553
<a id="ERR_METHOD_NOT_IMPLEMENTED"></a>
15431554
### `ERR_METHOD_NOT_IMPLEMENTED`
15441555

@@ -2534,6 +2545,7 @@ such as `process.stdout.on('data')`.
25342545
[`Class: assert.AssertionError`]: assert.html#assert_class_assert_assertionerror
25352546
[`ERR_INVALID_ARG_TYPE`]: #ERR_INVALID_ARG_TYPE
25362547
[`EventEmitter`]: events.html#events_class_eventemitter
2548+
[`MessagePort`]: worker_threads.html#worker_threads_class_messageport
25372549
[`Object.getPrototypeOf`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getPrototypeOf
25382550
[`Object.setPrototypeOf`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/setPrototypeOf
25392551
[`REPL`]: repl.html

doc/api/worker_threads.md

+18
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,15 @@ input of [`port.postMessage()`][].
303303
Listeners on this event will receive a clone of the `value` parameter as passed
304304
to `postMessage()` and no further arguments.
305305

306+
### Event: `'messageerror'`
307+
<!-- YAML
308+
added: REPLACEME
309+
-->
310+
311+
* `error` {Error} An Error object
312+
313+
The `'messageerror'` event is emitted when deserializing a message failed.
314+
306315
### `port.close()`
307316
<!-- YAML
308317
added: v10.5.0
@@ -683,6 +692,15 @@ See the [`port.on('message')`][] event for more details.
683692
All messages sent from the worker thread will be emitted before the
684693
[`'exit'` event][] is emitted on the `Worker` object.
685694

695+
### Event: `'messageerror'`
696+
<!-- YAML
697+
added: REPLACEME
698+
-->
699+
700+
* `error` {Error} An Error object
701+
702+
The `'messageerror'` event is emitted when deserializing a message failed.
703+
686704
### Event: `'online'`
687705
<!-- YAML
688706
added: v10.5.0

lib/internal/worker.js

+3-1
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,9 @@ class Worker extends EventEmitter {
191191
transferList.push(...options.transferList);
192192

193193
this[kPublicPort] = port1;
194-
this[kPublicPort].on('message', (message) => this.emit('message', message));
194+
for (const event of ['message', 'messageerror']) {
195+
this[kPublicPort].on(event, (message) => this.emit(event, message));
196+
}
195197
setupPortReferencing(this[kPublicPort], this, 'message');
196198
this[kPort].postMessage({
197199
argv,

src/env.h

+2
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,7 @@ constexpr size_t kFsStatsBufferLength =
231231
V(done_string, "done") \
232232
V(duration_string, "duration") \
233233
V(ecdh_string, "ECDH") \
234+
V(emit_string, "emit") \
234235
V(emit_warning_string, "emitWarning") \
235236
V(empty_object_string, "{}") \
236237
V(encoding_string, "encoding") \
@@ -288,6 +289,7 @@ constexpr size_t kFsStatsBufferLength =
288289
V(message_port_constructor_string, "MessagePort") \
289290
V(message_port_string, "messagePort") \
290291
V(message_string, "message") \
292+
V(messageerror_string, "messageerror") \
291293
V(minttl_string, "minttl") \
292294
V(module_string, "module") \
293295
V(modulus_string, "modulus") \

src/node_errors.h

+4
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ void OnFatalError(const char* location, const char* message);
4242
V(ERR_INVALID_ARG_TYPE, TypeError) \
4343
V(ERR_INVALID_TRANSFER_OBJECT, TypeError) \
4444
V(ERR_MEMORY_ALLOCATION_FAILED, Error) \
45+
V(ERR_MESSAGE_TARGET_CONTEXT_UNAVAILABLE, Error) \
4546
V(ERR_MISSING_ARGS, TypeError) \
4647
V(ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST, TypeError) \
4748
V(ERR_MISSING_PASSPHRASE, TypeError) \
@@ -96,6 +97,9 @@ void OnFatalError(const char* location, const char* message);
9697
V(ERR_INVALID_TRANSFER_OBJECT, "Found invalid object in transferList") \
9798
V(ERR_MEMORY_ALLOCATION_FAILED, "Failed to allocate memory") \
9899
V(ERR_OSSL_EVP_INVALID_DIGEST, "Invalid digest used") \
100+
V(ERR_MESSAGE_TARGET_CONTEXT_UNAVAILABLE, \
101+
"A message object could not be deserialized successfully in the target " \
102+
"vm.Context") \
99103
V(ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST, \
100104
"Object that needs transfer was found in message but not listed " \
101105
"in transferList") \

src/node_messaging.cc

+22-3
Original file line numberDiff line numberDiff line change
@@ -772,7 +772,17 @@ void MessagePort::OnMessage() {
772772
Local<Function> emit_message = PersistentToLocal::Strong(emit_message_fn_);
773773

774774
Local<Value> payload;
775-
if (!ReceiveMessage(context, true).ToLocal(&payload)) goto reschedule;
775+
Local<Value> message_error;
776+
{
777+
// Catch any exceptions from parsing the message itself (not from
778+
// emitting it) as 'messageeror' events.
779+
TryCatchScope try_catch(env());
780+
if (!ReceiveMessage(context, true).ToLocal(&payload)) {
781+
if (try_catch.HasCaught() && !try_catch.HasTerminated())
782+
message_error = try_catch.Exception();
783+
goto reschedule;
784+
}
785+
}
776786
if (payload == env()->no_message_symbol()) break;
777787

778788
if (!env()->can_call_into_js()) {
@@ -783,6 +793,16 @@ void MessagePort::OnMessage() {
783793

784794
if (MakeCallback(emit_message, 1, &payload).IsEmpty()) {
785795
reschedule:
796+
if (!message_error.IsEmpty()) {
797+
// This should become a `messageerror` event in the sense of the
798+
// EventTarget API at some point.
799+
Local<Value> argv[] = {
800+
env()->messageerror_string(),
801+
message_error
802+
};
803+
USE(MakeCallback(env()->emit_string(), arraysize(argv), argv));
804+
}
805+
786806
// Re-schedule OnMessage() execution in case of failure.
787807
if (data_)
788808
TriggerAsync();
@@ -1245,8 +1265,7 @@ BaseObjectPtr<BaseObject> JSTransferable::Data::Deserialize(
12451265
// the end of the stream, after the main message has been read.
12461266

12471267
if (context != env->context()) {
1248-
// It would be nice to throw some kind of exception here, but how do we
1249-
// pass that to end users? For now, just drop the message silently.
1268+
THROW_ERR_MESSAGE_TARGET_CONTEXT_UNAVAILABLE(env);
12501269
return {};
12511270
}
12521271
HandleScope handle_scope(env->isolate());

test/parallel/test-worker-message-port-transfer-fake-js-transferable-internal.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ const { once } = require('events');
2626
port1.postMessage(fh, [ fh ]);
2727
port2.on('message', common.mustNotCall());
2828

29-
const [ exception ] = await once(process, 'uncaughtException');
29+
const [ exception ] = await once(port2, 'messageerror');
3030

3131
assert.strictEqual(exception.message, 'Unknown deserialize spec net:Socket');
3232
port2.close();

test/parallel/test-worker-message-port-transfer-fake-js-transferable.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ module.exports = {
3030
port1.postMessage(fh, [ fh ]);
3131
port2.on('message', common.mustNotCall());
3232

33-
const [ exception ] = await once(process, 'uncaughtException');
33+
const [ exception ] = await once(port2, 'messageerror');
3434

3535
assert.match(exception.message, /Missing internal module/);
3636
port2.close();

test/parallel/test-worker-message-port-transfer-filehandle.js

+6
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,12 @@ const { once } = require('events');
5555
assert.strictEqual(msgEvent.data, 'second message');
5656
port1.close();
5757
});
58+
// TODO(addaleax): Switch this to a 'messageerror' event once MessagePort
59+
// implements EventTarget fully and in a cross-context manner.
60+
port2moved.emit = common.mustCall((name, err) => {
61+
assert.strictEqual(name, 'messageerror');
62+
assert.strictEqual(err.code, 'ERR_MESSAGE_TARGET_CONTEXT_UNAVAILABLE');
63+
});
5864
port2moved.start();
5965

6066
assert.notStrictEqual(fh.fd, -1);

0 commit comments

Comments
 (0)