src: add error message if we abort #8634

fhinkel · 2016-09-17T17:02:06Z

Checklist

make -j4 test (UNIX) passes
commit message follows commit guidelines

Affected core subsystem(s)

vm, watchdog

Description of change

Add an error message if we abort because uv_init_loop fails.

Fixes #8555

targos · 2016-09-17T17:06:28Z

src/node_watchdog.cc

@@ -13,6 +13,10 @@ Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms) : isolate_(isolate),
  loop_ = new uv_loop_t;
  CHECK(loop_);
  rc = uv_loop_init(loop_);
+  if (rc != 0) {
+       FatalError("node::Watchdog::Watchdog()",


style: I think indentation should be 2 spaces here

Thanks for catching it! I keep forgetting that ESlint doesn't complain about C++ formatting. Does anybody want to share their vimrc with me for correct auto formatting?

targos · 2016-09-17T17:07:31Z

src/node_watchdog.cc

@@ -13,6 +13,10 @@ Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms) : isolate_(isolate),
  loop_ = new uv_loop_t;
  CHECK(loop_);
  rc = uv_loop_init(loop_);
+  if (rc != 0) {
+       FatalError("node::Watchdog::Watchdog()",
+               "Failed to initialize loop. Maybe the file limit is reached?");


style: align with previous argument ?

cjihrig · 2016-09-17T17:06:52Z

src/node_watchdog.cc

@@ -13,6 +13,10 @@ Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms) : isolate_(isolate),
  loop_ = new uv_loop_t;
  CHECK(loop_);
  rc = uv_loop_init(loop_);
+  if (rc != 0) {
+       FatalError("node::Watchdog::Watchdog()",


The indentation looks off here.

cjihrig · 2016-09-17T17:08:38Z

src/node_watchdog.cc

@@ -13,6 +13,10 @@ Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms) : isolate_(isolate),
  loop_ = new uv_loop_t;
  CHECK(loop_);
  rc = uv_loop_init(loop_);
+  if (rc != 0) {
+       FatalError("node::Watchdog::Watchdog()",
+               "Failed to initialize loop. Maybe the file limit is reached?");


I'm not sure about the second part of this error message. If we can't say with certainty what the problem is, I don't think we should leave questions like this.

Maybe we could then check if the file limit was actually reached?

I was trying to make the message more helpful than just saying that the call failed. But I'm also OK with deleting the second part.

Should it say "initialize uv loop"? To give more context?

Maybe we could then check if the file limit was actually reached?

That would be pretty nice. I’m not sure whether that’s feasible to do in a cross-platform way, but for POSIXes it probably would work.

Yeah, I think @addaleax is right that it may be difficult to do x-platform reliably. The message could be extended a bit with something like, Failed to initialize loop. This may be caused, for instance, by reaching the file limit.

targos · 2016-09-17T17:22:47Z

src/node_watchdog.cc

@@ -13,6 +13,10 @@ Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms) : isolate_(isolate),
  loop_ = new uv_loop_t;
  CHECK(loop_);
  rc = uv_loop_init(loop_);
+  if (rc != 0) {
+     FatalError("node::Watchdog::Watchdog()",


sorry to bother you, but indentation is still off by 1.

No problem, my fault for being sloppy.

imyller · 2016-09-17T23:53:29Z

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

cjihrig

LGTM pending CI

imyller

LGTM

imyller · 2016-09-18T09:50:37Z

Oh, @thealphanerd do you feel citgm should be run before landing this?

addaleax

LGTM but the commit message should start with vm: or maybe src:?

(I don’t think CITGM is really necessary here, but if somebody wants to run it, sure)

addaleax · 2016-09-18T09:57:25Z

src/node_watchdog.cc

+  if (rc != 0) {
+    FatalError("node::Watchdog::Watchdog()",
+               "Failed to initialize uv loop.");
+  }
  CHECK_EQ(0, rc);


I think this line can be dropped.

Add an error message in watchdog if we abort because uv_loop_init fails. Fixes nodejs#8555

fhinkel · 2016-09-20T09:33:28Z

I fixed the formatting, error message, extra CHECK, and commit message.

CI again: https://ci.nodejs.org/view/All/job/node-test-pull-request/4146/

jasnell · 2016-09-20T18:41:31Z

I don't believe it needs to be but should this be semver-major?

cjihrig · 2016-09-20T22:21:42Z

I don't think this should be semver major since it's an abort. I'd go with patch.

jasnell

LGTM

imyller · 2016-09-22T22:13:06Z

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

imyller · 2016-09-23T00:05:19Z

I'll start landing this:

Four LGTMs
No objections
Requested modifications have been made
CI tests passed (only known CI failures; unrelated to this PR)

Add an error message in watchdog if we abort because uv_loop_init fails. PR-URL: nodejs#8634 Fixes: nodejs#8555 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

imyller · 2016-09-23T00:13:49Z

landed in fba5319

Thank you, @fhinkel

Add an error message in watchdog if we abort because uv_loop_init fails. PR-URL: #8634 Fixes: #8555 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2016-10-06T23:14:18Z

@fhinkel do you think this should be backported?

Add an error message in watchdog if we abort because uv_loop_init fails. PR-URL: #8634 Fixes: #8555 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2016-11-18T20:40:04Z

ping @fhemberger

fhemberger · 2016-11-19T05:19:06Z

I guess you wanted to ping @fhinkel. 😄

MylesBorins · 2016-12-21T19:59:58Z

ping @fhinkel

fhinkel · 2016-12-21T20:29:58Z

If it lands cleanly, yes.

Add an error message in watchdog if we abort because uv_loop_init fails. PR-URL: #8634 Fixes: #8555 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 17, 2016

fhinkel changed the title ~~src: better error message on uv_loop_init failure~~ Watchdog: add error message if we abort Sep 17, 2016

targos reviewed Sep 17, 2016

View reviewed changes

cjihrig reviewed Sep 17, 2016

View reviewed changes

fhinkel force-pushed the i8555 branch from 33e05fa to 7383233 Compare September 17, 2016 17:09

targos reviewed Sep 17, 2016

View reviewed changes

fhinkel force-pushed the i8555 branch 2 times, most recently from 892de33 to 7fe7b13 Compare September 17, 2016 17:32

addaleax added the vm Issues and PRs related to the vm subsystem. label Sep 17, 2016

cjihrig approved these changes Sep 17, 2016

View reviewed changes

imyller approved these changes Sep 18, 2016

View reviewed changes

addaleax approved these changes Sep 18, 2016

View reviewed changes

vm: add error message if we abort

a1e3f7b

Add an error message in watchdog if we abort because uv_loop_init fails. Fixes nodejs#8555

fhinkel force-pushed the i8555 branch from 7fe7b13 to a1e3f7b Compare September 20, 2016 09:32

fhinkel changed the title ~~Watchdog: add error message if we abort~~ src: add error message if we abort Sep 20, 2016

jasnell added the error-handling label Sep 20, 2016

jasnell approved these changes Sep 20, 2016

View reviewed changes

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:10

imyller self-assigned this Sep 22, 2016

imyller closed this Sep 23, 2016

imyller removed their assignment Sep 23, 2016

MylesBorins added the lts-watch-v4.x label Oct 6, 2016

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 23, 2017

MylesBorins mentioned this pull request Jan 24, 2017

v4.8.0 proposal #10973

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: add error message if we abort #8634

src: add error message if we abort #8634

fhinkel commented Sep 17, 2016 •

edited

Loading

targos Sep 17, 2016 •

edited

Loading

fhinkel Sep 17, 2016

targos Sep 17, 2016

cjihrig Sep 17, 2016

cjihrig Sep 17, 2016

Fishrock123 Sep 17, 2016

fhinkel Sep 17, 2016

fhinkel Sep 17, 2016

addaleax Sep 17, 2016

jasnell Sep 20, 2016

targos Sep 17, 2016

fhinkel Sep 17, 2016

imyller commented Sep 17, 2016

cjihrig left a comment

imyller left a comment

imyller commented Sep 18, 2016

addaleax left a comment

addaleax Sep 18, 2016

fhinkel commented Sep 20, 2016 •

edited

Loading

jasnell commented Sep 20, 2016

cjihrig commented Sep 20, 2016

jasnell left a comment

imyller commented Sep 22, 2016

imyller commented Sep 23, 2016

imyller commented Sep 23, 2016

MylesBorins commented Oct 6, 2016

MylesBorins commented Nov 18, 2016

fhemberger commented Nov 19, 2016

MylesBorins commented Dec 21, 2016

fhinkel commented Dec 21, 2016

src: add error message if we abort #8634

src: add error message if we abort #8634

Conversation

fhinkel commented Sep 17, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

targos Sep 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imyller commented Sep 17, 2016

cjihrig left a comment

Choose a reason for hiding this comment

imyller left a comment

Choose a reason for hiding this comment

imyller commented Sep 18, 2016

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhinkel commented Sep 20, 2016 • edited Loading

jasnell commented Sep 20, 2016

cjihrig commented Sep 20, 2016

jasnell left a comment

Choose a reason for hiding this comment

imyller commented Sep 22, 2016

imyller commented Sep 23, 2016

imyller commented Sep 23, 2016

MylesBorins commented Oct 6, 2016

MylesBorins commented Nov 18, 2016

fhemberger commented Nov 19, 2016

MylesBorins commented Dec 21, 2016

fhinkel commented Dec 21, 2016

fhinkel commented Sep 17, 2016 •

edited

Loading

targos Sep 17, 2016 •

edited

Loading

fhinkel commented Sep 20, 2016 •

edited

Loading