@nodejs/cpp-reviewers

@RaisinTen

We should also change all call sites of the functions modified in this PR to do the needful based on the return values.

This seems to be the only call site for OnUvRead().

I tried to do the needful at the call site but couldn't make it up as the result. The followings are what I tried.
Could you take a look?

• I triggered the onerror js function if the IsNothing() returns true. In this case, some tests fail or hang if I don't use a verbose TryCatch scope.

#include "node_errors.h"
...
using errors::TryCatchScope;
using v8::Isolate;
...

[](uv_stream_t* stream, ssize_t nread, const uv_buf_t* buf) {
{
    LibuvStreamWrap* wrap = static_cast<LibuvStreamWrap*>(stream->data);
    Environment* env = wrap->env();
    Isolate* isolate = env->isolate();

    HandleScope handle_scope(isolate);
    Context::Scope context_scope(env->context());

    TryCatchScope try_catch(env);
    // try_catch.SetVerbose(true); // IIUC, this isn't needed but, without this, some tests fail.
    
    if (wrap->OnUvRead(nread, buf).IsNothing()) {
      DCHECK(try_catch.HasCaught() && !try_catch.HasTerminated());
      Local<Value> argv[] = {
          v8::Integer::New(isolate, static_cast<int32_t>(nread)),
          wrap->GetObject(),
          try_catch.Exception()
      };
      wrap->MakeCallback(env->onerror_string(), arraysize(argv), argv);
    }
}

• I just called SetVerbose(true) and expected PerIsolateMessageListener() would be invoked to trigger the uncaughtException event. However, I couldn't receive the event when I forcibly tried ThrowErrorException() in OnUvRead().

[](uv_stream_t* stream, ssize_t nread, const uv_buf_t* buf) {
{
    LibuvStreamWrap* wrap = static_cast<LibuvStreamWrap*>(stream->data);
    TryCatchScope try_catch(wrap->env());
    try_catch.SetVerbose(true);
    wrap->OnUvRead(nread, buf);
}

• If I manually called errors::TriggerUncaughtException(), then I could receive the uncaughtException event. However, the verbose TryCatch scope is required to get the tests passed.

[](uv_stream_t* stream, ssize_t nread, const uv_buf_t* buf) {
{
    LibuvStreamWrap* wrap = static_cast<LibuvStreamWrap*>(stream->data);
    TryCatchScope try_catch(wrap->env());
    // try_catch.SetVerbose(true); // IIUC, this shouldn't be called but, without this, some tests fail.

    if (wrap->OnUvRead(nread, buf).IsNothing()) {
      if (try_catch.HasCaught() && !try_catch.HasTerminated())
          errors::TriggerUncaughtException(wrap->env()->isolate(), try_catch);
    }
}

• I triggered the onerror js function if the IsNothing() returns true. In this case, some tests fail or hang if I don't use a verbose TryCatch scope.

Are you sure that the JS-side of the object uses an onerror() callback to report errors to the public API?
I think there might be another problem because of running MakeCallback() when an exception has already been thrown. Maybe we should call MakeCallback in a scope where the TryCatchScope object isn't present on the stack like I did in #42054?

Are you sure that the JS-side of the object uses an onerror() callback to report errors to the public API?

On second thought, onerror() callback couldn't be used for reporting errors in the LibuvStreamWrap perspective since the JS-side of the object may not use it. Then, using the uncaught exception seems to be an alternative way.

@RaisinTen Updated the call site using a verbose TryCatch scope, PTAL.

I had also considered manually triggering the uncaught exception as below. However, after reading #42054 (comment), I noticed SetVerbose(true) would be enough. The verbose scope would work if actual throwing exceptions occur in OnUvRead().

LibuvStreamWrap* wrap = static_cast<LibuvStreamWrap*>(stream->data);
TryCatchScope try_catch(wrap->env());
try_catch.SetVerbose(true);
if (wrap->OnUvRead(nread, buf).IsNothing()) {
  if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
    try_catch.SetVerbose(false);
    errors::TriggerUncaughtException(wrap->env()->isolate(), try_catch);
  }
}

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

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

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

Landed in 141052d