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

report: do not use uv_default_loop() as fallback #25652

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Not seeing an associated Environment is a rare condition anyway,
but using uv_default_loop() as a fallback is not thread-safe.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 23, 2019
@addaleax addaleax added the report Issues and PRs related to process.report. label Jan 23, 2019
@addaleax
Copy link
Member Author

@@ -307,8 +307,6 @@ static void WriteNodeReport(Isolate* isolate,
writer.json_arraystart("libuv");
if (env != nullptr)
uv_walk(env->event_loop(), WalkHandle, static_cast<void*>(&writer));
else
uv_walk(uv_default_loop(), WalkHandle, static_cast<void*>(&writer));
Copy link
Member

Choose a reason for hiding this comment

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

Since the report is meant to aid diagnostics, is there any value in logging that env was null in the report?

Copy link
Member Author

Choose a reason for hiding this comment

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

@richardlau I guess we could do that – do you have any suggestion on how to represent that?

@gireeshpunathil
Copy link
Member

@addaleax :

Not seeing an associated Environment is a rare condition anyway,

IIRC, under fatalerror conditions (as registered to, detected and called back by v8) env was always null; either:

  • somewhere on the line the env pointer is nullified by someone, or
  • the fatalerror handler is invoked in such a scenario where Environment and Isolate are disassociated.

Something to investigate in future, but irrespective of that, this change looks good to me, for a clean and meaningful report.

@addaleax
Copy link
Member Author

@gireeshpunathil I think the test that checks fatalerror conditions is OOM-based, so, in that case no Environment would be associated with the Isolate because the Isolate is running the garbage collector when it fails?

I think for other fatal errors (e.g. calling ToLocalChecked() on an empty MaybeLocal<>) that’s not the case:

$ ./node --experimental-report --diagnostic-report-on-fatalerror
> Object.defineProperty(Object.prototype, 'Serializer', { set() { throw new Error('BOOM'); } }) 
{}
> require('v8')
FATAL ERROR: v8::FromJust Maybe value is Nothing.

Writing Node.js report to file: report.20190123.145041.2475.001.json
Node.js report completed
[...]
$ grep -A5 libuv report.20190123.145041.2475.001.json 
  "libuv": [
    {
      "type": "async",
      "is_active": "1",
      "is_referenced": "0",
      "address": "93930778479248",

@gireeshpunathil
Copy link
Member

@addaleax - thanks for the clarification, I wasn't aware of such fatalerror types.

Not seeing an associated `Environment` is a rare condition anyway,
but using `uv_default_loop()` as a fallback is not thread-safe.
@addaleax addaleax force-pushed the report-no-uv-default-loop branch from 0bfd3bc to 0542913 Compare January 24, 2019 18:43
@addaleax addaleax changed the title report: do not blindly use uv_default_loop() report: do not use uv_default_loop() as fallback Jan 24, 2019
@addaleax
Copy link
Member Author

(If you’re wondering why I changed the commit message, it just occurred to me that the previous one is kind of ableist.)

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 24, 2019
@addaleax
Copy link
Member Author

@Trott
Copy link
Member

Trott commented Jan 26, 2019

@addaleax
Copy link
Member Author

Landed in bb774b1

@addaleax addaleax closed this Jan 27, 2019
@addaleax addaleax deleted the report-no-uv-default-loop branch January 27, 2019 15:24
addaleax added a commit that referenced this pull request Jan 27, 2019
Not seeing an associated `Environment` is a rare condition anyway,
but using `uv_default_loop()` as a fallback is not thread-safe.

PR-URL: #25652
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
addaleax added a commit that referenced this pull request Jan 28, 2019
Not seeing an associated `Environment` is a rare condition anyway,
but using `uv_default_loop()` as a fallback is not thread-safe.

PR-URL: #25652
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@targos targos mentioned this pull request Jan 29, 2019
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++. report Issues and PRs related to process.report.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants