src: add error message if we abort by fhinkel · Pull Request #8634 · nodejs/node

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

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

style: align with previous argument ?

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

The indentation looks off here.

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

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

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

I think this line can be dropped.

Add an error message in watchdog if we abort because uv_loop_init fails. Fixesnodejs#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#8634Fixes: 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: #8634Fixes: #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: #8634Fixes: #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: #8634Fixes: #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 abortSep 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 7383233Compare 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 7fe7b13Compare 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

Comment threadsrc/node_watchdog.cc Outdated
addaleaxSep 18, 2016
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line can be dropped.

vm: add error message if we abort
a1e3f7b
Add an error message in watchdog if we abort because uv_loop_init fails. Fixesnodejs#8555

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

fhinkel changed the title ~~Watchdog: add error message if we abort~~src: add error message if we abortSep 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 c5ce7f4Compare 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

Uh oh!

Uh oh!

Conversation

fhinkel commented Sep 17, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

targosSep 17, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imyller commented Sep 17, 2016

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

imyller left a comment

Choose a reason for hiding this comment

Uh oh!

imyller commented Sep 18, 2016

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhinkel commented Sep 20, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Sep 20, 2016

Uh oh!

cjihrig commented Sep 20, 2016

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

imyller commented Sep 22, 2016

Uh oh!

imyller commented Sep 23, 2016

Uh oh!

imyller commented Sep 23, 2016

Uh oh!

MylesBorins commented Oct 6, 2016

Uh oh!

MylesBorins commented Nov 18, 2016

Uh oh!

fhemberger commented Nov 19, 2016

Uh oh!

MylesBorins commented Dec 21, 2016

Uh oh!

fhinkel commented Dec 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

fhinkel commented Sep 17, 2016•
edited
Loading

targosSep 17, 2016•
edited
Loading

fhinkel commented Sep 20, 2016•
edited
Loading