Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src,stream: change return type to Maybe #43575

Merged
merged 4 commits into from
Jul 7, 2022

Conversation

daeyeon
Copy link
Member

@daeyeon daeyeon commented Jun 26, 2022

This changes the return types of some functions to indicate that the functions may have a pending exception.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 26, 2022
@daeyeon daeyeon force-pushed the main.stream-220626.Sun.d853 branch 3 times, most recently from fcc16f5 to cc16070 Compare June 26, 2022 11:21
@targos
Copy link
Member

targos commented Jun 26, 2022

@nodejs/cpp-reviewers

src/stream_wrap.cc Outdated Show resolved Hide resolved
src/stream_wrap.cc Outdated Show resolved Hide resolved
src/stream_base-inl.h Outdated Show resolved Hide resolved
This changes the return types of some functions to indicate that
the functions may have a pending exception, and removes some of
todos related.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
@daeyeon daeyeon force-pushed the main.stream-220626.Sun.d853 branch from a0d6999 to ed77f33 Compare June 28, 2022 01:03
@daeyeon
Copy link
Member Author

daeyeon commented Jun 28, 2022

@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().

node/src/stream_wrap.cc

Lines 200 to 203 in 7cbcc4f

}, [](uv_stream_t* stream, ssize_t nread, const uv_buf_t* buf) {
static_cast<LibuvStreamWrap*>(stream->data)->OnUvRead(nread, buf);
});
}

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);
    }
}

@daeyeon daeyeon force-pushed the main.stream-220626.Sun.d853 branch from ed77f33 to 2c0c91b Compare June 28, 2022 03:50
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
@daeyeon daeyeon force-pushed the main.stream-220626.Sun.d853 branch from 2c0c91b to 83cafe8 Compare June 28, 2022 09:56
@RaisinTen
Copy link
Contributor

RaisinTen commented Jun 28, 2022

  • 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?

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
@daeyeon
Copy link
Member Author

daeyeon commented Jun 29, 2022

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);
  }
}

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nodejs-github-bot
Copy link
Collaborator

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 29, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 7, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 7, 2022
@nodejs-github-bot nodejs-github-bot merged commit 141052d into nodejs:main Jul 7, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 141052d

@daeyeon daeyeon deleted the main.stream-220626.Sun.d853 branch July 7, 2022 23:19
targos pushed a commit that referenced this pull request Jul 12, 2022
This changes the return types of some functions to indicate that
the functions may have a pending exception, and removes some of
todos related.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #43575
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Jul 20, 2022
This changes the return types of some functions to indicate that
the functions may have a pending exception, and removes some of
todos related.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #43575
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Jul 31, 2022
This changes the return types of some functions to indicate that
the functions may have a pending exception, and removes some of
todos related.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #43575
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
This changes the return types of some functions to indicate that
the functions may have a pending exception, and removes some of
todos related.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: nodejs/node#43575
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants