src: properly manage timer in cares ChannelWrap by addaleax · Pull Request #14634 · nodejs/node · GitHub

This fixes a bug introduced in 727b291 where code managing the uv_timer_t for a ChannelWrap instance was left unchanged, when it should have changed the lifetime of the handle to being tied to the ChannelWrap instance’s lifetime.

Fixes: #14599
Ref: #14518

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included (eeeeh … in the form of an existing test that was flaky)
commit message follows commit guidelines

Affected core subsystem(s)

src/cares_wrap

This fixes a bug introduced in 727b291 where code managing the `uv_timer_t` for a `ChannelWrap` instance was left unchanged, when it should have changed the lifetime of the handle to being tied to the `ChannelWrap` instance’s lifetime. Fixes: nodejs#14599 Ref: nodejs#14518

CI: https://ci.nodejs.org/job/node-test-commit/11555/ (edit: green except infra failure)
Stress master: https://ci.nodejs.org/job/node-stress-single-test/1358/ (edit: red, 245/1000 failed)
Stress this PR: https://ci.nodejs.org/job/node-stress-single-test/1359/ (edit: green, 0/1000 failed)

It would be cool to have this in 8.3.0, this fixes a real crash.

edit: I won’t be here for most of the weekend, please feel free to fix any change requests yourself

Is it possible to add a test?

@mscdex I don’t think that would be any more or less reliable than the existing failing test.

Also, this isn’t about the set* globals/the timers core module that the label is for, so I think that can be removed?

Given the frequency of crashes in CI on test-async-wrap-getasyncid, it would be great to fast-track this. But of course, first it needs some reviews... 📟 @bnoordhuis @trevnorris @indutny

+1 to fast track after it gets a review from the aforementioned

LGTM with minor nits.

Double newline here, please.

Not absolutely necessary, but doing handle->data = NULL prior to this may help in the future.

New CI: https://ci.nodejs.org/job/node-test-commit/11568/
New Stress: https://ci.nodejs.org/job/node-stress-single-test/1360/
(yay "Rebuild")

Yikes, nullptr 😉 Sorry!

Sorry I'm doing this "blind" on the GitHub GUI.

New stress: https://ci.nodejs.org/job/node-stress-single-test/1361/ https://ci.nodejs.org/job/node-stress-single-test/1362/nodes=aix61-ppc64/
New CI: https://ci.nodejs.org/job/node-test-commit/11569/

ARM fail is #14206
So CI & stress are ✔️

LGTM. But IMHO why don't we use CARE's own timeout?

Refs: c-ares/c-ares#135 (comment)

@XadillaX As far as I can tell, c-ares is not involved in actually creating timers in any way.

LGTM modulo nits.

if (timer_handle_ == nullptr) return;

Dead store; the memory is deleted immediately afterwards.

Right, this was requested by @indutny and added by @refack. If they had any particular reason for that, please speak up, but I’ve removed it again for now.

Landed in 41a0dfc

This fixes a bug introduced in 727b291 where code managing the `uv_timer_t` for a `ChannelWrap` instance was left unchanged, when it should have changed the lifetime of the handle to being tied to the `ChannelWrap` instance’s lifetime. Fixes: #14599 Ref: #14518 PR-URL: #14634 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

addaleax requested a review from XadillaX August 4, 2017 19:01

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. labels Aug 4, 2017

addaleax mentioned this pull request Aug 4, 2017

test/parallel/test-async-wrap-getasyncid.js crashing #14599

Closed

mscdex added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Aug 4, 2017

refack approved these changes Aug 5, 2017

View reviewed changes

jasnell approved these changes Aug 5, 2017

View reviewed changes

tniessen approved these changes Aug 5, 2017

View reviewed changes

indutny approved these changes Aug 5, 2017

View reviewed changes

addressing indutny comments

e764842

indutny reviewed Aug 5, 2017

View reviewed changes

nullptr

1fdf0ac

XadillaX approved these changes Aug 6, 2017

View reviewed changes

bnoordhuis approved these changes Aug 7, 2017

View reviewed changes

addaleax added 2 commits August 7, 2017 12:42

[squash] undo pointless change

75885f3

[squash] style nit

8a19e57

addaleax mentioned this pull request Aug 7, 2017

v8.3.0 proposal #14594

Merged

addaleax closed this Aug 7, 2017

addaleax deleted the cares-fix-timer branch August 7, 2017 11:59

MylesBorins added lts-watch-v6.x baking-for-lts PRs that need to wait before landing in a LTS release. labels Aug 16, 2017

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

                 ares_destroy(channel_);
+                CleanupTimer();
+              }

+                uv_close(reinterpret_cast<uv_handle_t*>(timer_handle_),
+                         [](uv_handle_t* handle) {
+                  delete reinterpret_cast<uv_timer_t*>(handle);



		void ChannelWrap::CleanupTimer() {
		if (!timer_handle_) return;

src: properly manage timer in cares ChannelWrap by addaleax · Pull Request #14634 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: properly manage timer in cares ChannelWrap - #14634

src: properly manage timer in cares ChannelWrap#14634addaleax wants to merge 5 commits intonodejs:masternodejs/node:masterfrom addaleax:cares-fix-timeraddaleax/node:cares-fix-timerCopy head branch name to clipboard

Conversation

addaleax commented Aug 4, 2017

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Aug 4, 2017

Uh oh!

addaleax commented Aug 4, 2017

Uh oh!

Trott commented Aug 5, 2017

Uh oh!

refack commented Aug 5, 2017

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

indutny Aug 5, 2017

Choose a reason for hiding this comment

Uh oh!

indutny Aug 5, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 5, 2017

Uh oh!

indutny Aug 5, 2017

Choose a reason for hiding this comment

Uh oh!

refack Aug 5, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Aug 5, 2017

Uh oh!

XadillaX left a comment • 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!

addaleax commented Aug 7, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Aug 7, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Aug 7, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Aug 7, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax commented Aug 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

src: properly manage timer in cares ChannelWrap#14634
addaleax wants to merge 5 commits into
nodejs:masterfrom
addaleax:cares-fix-timer

addaleax commented Aug 4, 2017 •
edited
Loading

refack commented Aug 5, 2017 •
edited
Loading

XadillaX left a comment •
edited
Loading