src: properly report exceptions from AddressToJS()

[RaisinTen]

Signed-off-by: Darshan Sen [email protected]

cc @addaleax

[nodejs-github-bot]

Review requested:

• @nodejs/net

[RaisinTen]

@addaleax PTAL

[addaleax]

You can just return; if the MaybeLocal is empty here. The TryCatchScope will have called the Isolate’s message handler, i.e. have resulted in an uncaughtException event being emitted.

[RaisinTen]

Do you know where the TryCatchScope is supposed to call the handler? I tried doing it for the above block but it doesn't emit the uncaughtException event on the process object (if that's what you mean?). It just hangs.

diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc
index 0fb15ce0a0..8a850d1a01 100644
--- a/src/udp_wrap.cc
+++ b/src/udp_wrap.cc
@@ -735,13 +735,16 @@ void UDPWrap::OnRecv(ssize_t nread,
     TryCatchScope try_catch(env);
     try_catch.SetVerbose(true);
     DCHECK(try_catch.IsVerbose());
-    if (!AddressToJS(env, addr).ToLocal(&address)) {
+    env->ThrowError("AddressToJS error");
+    // if (!AddressToJS(env, addr).ToLocal(&address)) {
+      /*
       DCHECK(try_catch.HasCaught() && !try_catch.HasTerminated());
       argv[2] = try_catch.Exception();
       DCHECK(!argv[2].IsEmpty());
       MakeCallback(env->onerror_string(), arraysize(argv), argv);
+      */
       return;
-    }
+    // }
   }

   Local<ArrayBuffer> ab = ArrayBuffer::New(isolate, std::move(bs));
diff --git a/test/parallel/test-dgram-send-multi-string-array.js b/test/parallel/test-dgram-send-multi-string-array.js
index 8d73a6d183..b2db8c41b4 100644
--- a/test/parallel/test-dgram-send-multi-string-array.js
+++ b/test/parallel/test-dgram-send-multi-string-array.js
@@ -10,4 +10,8 @@ socket.on('message', common.mustCall((msg, rinfo) => {
   assert.deepStrictEqual(msg.toString(), data.join(''));
 }));

+socket.on('error', (err) => {
+  throw err;
+});
+
 socket.bind(() => socket.send(data, socket.address().port, 'localhost'));

[addaleax]

Do you know where the TryCatchScope is supposed to call the handler?

The .SetVerbose(true) should ensure that V8 does that.

It just hangs.

What does “hang” mean here?

[RaisinTen]

@addaleax

What does “hang” mean here?

Since the error doesn't get thrown the socket keeps the event loop alive and ./node test/parallel/test-dgram-send-multi-string-array.js just keeps running with no visible outcome.

[addaleax]

@RaisinTen Ah yeah … this is probably because ThrowError doesn’t actually throw exceptions from a V8 perspective, it just schedules them to be thrown in the future. If you run into an error that actually comes from here, you should be able to see the uncaught exception event being emitted.

[addaleax]

@addaleax are you thinking about something like

node/src/js_stream.cc

Lines 48 to 52 in 457567f

	if (!MakeCallback(env()->isclosing_string(), 0, nullptr).ToLocal(&value)) {
	if (try_catch.HasCaught() && !try_catch.HasTerminated())
	errors::TriggerUncaughtException(env()->isolate(), try_catch);
	return true;
	}

? I tried it out here but I'm not able to make it throw an uncaught exception. Still the same behaviour.

Yeah, I think errors::TriggerUncaughtException() would be what I was thinking of.

Does it mean that we need to first expose AddressToJS to JS and then call it from there when we run a MakeCallback here and finally call errors::TriggerUncaughtException(isolate, try_catch); with the result of the TryCatch?

To be clear, I don’t think we should do that, I just meant that in order for an exception to be thrown rather than schedule, it has to come from JS

I think that’s a choice that we can make, but we generally treat exceptions that happen in top-level callbacks as uncaught exceptions.

Yea, I don't think throwing an exception that cannot be caught is helpful to the user. To handle cases like these, it would mandate them to handle them at a global level even though there is a way to do so in the place closest to where the error gets thrown from.

I can see how that makes sense from a certain angle, but we should be consistent about what happens with exceptions that are emitted from top-level callbacks, and right now, these always end up as uncaught exceptions. And we can deviate from that for callbacks that are attached to EventEmitter objects, where we can emit 'error' events, but that’s not something we can easily do consistently between all of these types of EventEmitters, and even if we did, it would break consistency with other callbacks.

Is there anything else we need to do on this PR or does it look good to you?

We should not pass an exception from a verbose TryCatch back to userland again (i.e. in another way than through the message handler), so that’s something that would need to be taken care of either way.

[RaisinTen]

@addaleax

I can see how that makes sense from a certain angle, but we should be consistent about what happens with exceptions that are emitted from top-level callbacks, and right now, these always end up as uncaught exceptions.

I don't think the current change is making the error handling inconsistent from the rest of the dgram socket implementation. It already emits the 'error' event from a top level callback instead of throwing an uncaught exception -

node/lib/dgram.js

Lines 916 to 923 in a66b9ca

	function onMessage(nread, handle, buf, rinfo) {
	const self = handle[owner_symbol];
	if (nread < 0) {
	return self.emit('error', errnoException(nread, 'recvmsg'));
	}
	rinfo.size = buf.length; // compatibility
	self.emit('message', buf, rinfo);
	}

.

And we can deviate from that for callbacks that are attached to EventEmitter objects, where we can emit 'error' events,

+1 about handling errors in EventEmitters differently (i.e., the regular JS way) from other places where it's not really possible to do it this way. I'm okay with throwing uncaught exceptions from such places but I think we should also consider turning those into EventEmitters because it's more user friendly to handle errors of this sort.

but that’s not something we can easily do consistently between all of these types of EventEmitters, and even if we did, it would break consistency with other callbacks.

Why is that so?

We should not pass an exception from a verbose TryCatch back to userland again (i.e. in another way than through the message handler), so that’s something that would need to be taken care of either way.

I found a way to pass the error to userland without calling SetVerbose(true) on it and it works just like any other regular EventEmitter object implemented in pure JS - if an error is emitted and no handler is attached for the 'error' event, it causes the 'uncaughtException' event to be emitted on the process object, otherwise the error is passed to the handler and users may do anything they want with it (doesn't throw an uncaught exception).

The reason why this wasn't working before without the SetVerbose(true) call is that MakeCallback() doesn't actually call the callback if an exception is scheduled to be thrown from C++ during the call. The last commit fixes that by moving the MakeCallback() call after the TryCatchScope object goes out of scope.

PTAL

[addaleax]

+1 about handling errors in EventEmitters differently (i.e., the regular JS way) from other places where it's not really possible to do it this way. I'm okay with throwing uncaught exceptions from such places but I think we should also consider turning those into EventEmitters because it's more user friendly to handle errors of this sort.

That’s a big change to how Node.js works. Can we find a way to introduce this pattern in a place where it can receive broader review than in this PR?

We’re essentially saying that Node.js code that is equivalent to

function topLevelCallback() {
  <... internal node.js code ...>
  emitter.emit('event', ...)
}

should be transformed into

function topLevelCallback() {
  try {
    <... internal node.js code ...>
  } catch (err) {
    emitter.emit('error', err);
    return;
  }
  emitter.emit('event', ...)
}

(mod being possibly/partially written in C++ rather than JS)

but that’s not something we can easily do consistently between all of these types of EventEmitters, and even if we did, it would break consistency with other callbacks.

Why is that so?

Because it’s just a lot of work to introduce this for every single instance of this pattern – and that makes it dangerous to get stuck in a in-between state.

[RaisinTen]

@addaleax done - #42412

[RaisinTen]

@addaleax

I can see how that makes sense from a certain angle, but we should be consistent about what happens with exceptions that are emitted from top-level callbacks, and right now, these always end up as uncaught exceptions. And we can deviate from that for callbacks that are attached to EventEmitter objects, where we can emit 'error' events, but that’s not something we can easily do consistently between all of these types of EventEmitters, and even if we did, it would break consistency with other callbacks.

Could you please share some concrete EventEmitter API examples in the issue, where we get uncaught exceptions from top level callbacks? It was asked for in #42412 (comment) but I have never faced those before, so having that input should help in justifying if this is a big change.

[mcollina]

Can you add a test?

[RaisinTen]

@mcollina writing a test for this would require us to emulate the setup described in #41500. Do we really want to run the sequence of commands mentioned in the issue on CI?

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43188/

[nodejs-github-bot]

Landed in bc395d4