src: properly manage timer in cares ChannelWrap #14634

addaleax · 2017-08-04T19:01:23Z

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

addaleax · 2017-08-04T19:06:37Z

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

mscdex · 2017-08-04T19:31:13Z

Is it possible to add a test?

addaleax · 2017-08-04T19:35:43Z

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

Trott · 2017-08-05T14:39:31Z

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

refack · 2017-08-05T15:57:26Z

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

indutny

LGTM with minor nits.

indutny · 2017-08-05T17:48:07Z

src/cares_wrap.cc

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


Double newline here, please.

indutny · 2017-08-05T17:48:49Z

src/cares_wrap.cc

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


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

refack · 2017-08-05T17:57:29Z

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")

indutny · 2017-08-05T17:58:20Z

src/cares_wrap.cc

+
+  uv_close(reinterpret_cast<uv_handle_t*>(timer_handle_),
+           [](uv_handle_t* handle) {
+    handle->data = NULL;


Yikes, nullptr 😉 Sorry!

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

refack · 2017-08-05T18:01:04Z

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/

refack · 2017-08-05T19:26:21Z

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

XadillaX

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

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

addaleax · 2017-08-07T01:34:12Z

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

bnoordhuis

LGTM modulo nits.

bnoordhuis · 2017-08-07T10:29:06Z

src/cares_wrap.cc

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


if (timer_handle_ == nullptr) return;

bnoordhuis · 2017-08-07T10:32:00Z

src/cares_wrap.cc

+
+  uv_close(reinterpret_cast<uv_handle_t*>(timer_handle_),
+           [](uv_handle_t* handle) {
+    handle->data = nullptr;


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.

addaleax · 2017-08-07T11:58:48Z

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



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

Uh oh!

src: properly manage timer in cares ChannelWrap #14634

src: properly manage timer in cares ChannelWrap #14634

Uh oh!

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

addaleax commented Aug 4, 2017 •

edited

Loading

refack commented Aug 5, 2017 •

edited

Loading

XadillaX left a comment •

edited

Loading