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.

Sign up for GitHub

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

timers: fix not to close reused timer handle #11646

Closed
wants to merge 2 commits into from
Closed

timers: fix not to close reused timer handle #11646

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5462380
timers: fix not to close reused timer handle
shigeki Feb 20, 2017
f497955
test: descrease conuter to iterate
shigeki Mar 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion lib/timers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,6 @@ function listOnTimeout() {
// As such, we can remove the list and clean up the TimerWrap C++ handle.
debug('%d list empty', msecs);
assert(L.isEmpty(list));
this.close();

// Either refedLists[msecs] or unrefedLists[msecs] may have been removed and
// recreated since the reference to `list` was created. Make sure they're
Expand All @@ -232,6 +231,13 @@ function listOnTimeout() {
} else if (list === refedLists[msecs]) {
delete refedLists[msecs];
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it still work if this.close() was placed into both of these ifcases? I think that may be a better and more robust patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would work as far as I checked. I also made an additional test that has require('timers')._unrefActive(timer1) and found that the timer handle is not reused in that case. So it is okay.
Honestly, I'm not sure whether the following two conditions are equivalent or not. I think the latter is clearly understandable.

  if (list._unrefed === true && list === unrefedLists[msecs]) {
    delete unrefedLists[msecs];
    this.close();
  } else if (list === refedLists[msecs]) {
    delete refedLists[msecs];
    this.close();
  }
  // Do not close the underlying handle if its ownership has changed
  // (e.g it was unrefed in its callback).
  if (this.owner)
    return;

  this.close();

Copy link
Contributor

Choose a reason for hiding this comment

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

My thinking is that you only actually want to close the handle if the list was in a pool.

There are more obscure parts of the timers code imo so I don't think it is an issue.

Copy link

@misterdjules misterdjules Mar 3, 2017 •
edited by gibfahn
Loading

Choose a reason for hiding this comment

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

@Fishrock123

My thinking is that you only actually want to close the handle if the list was in a pool.

Do you mean that we want to close the TimerWrap handle only if the list is still in a pool?

Does that diff reflects the change you had in mind:

diff --git a/lib/timers.js b/lib/timers.js
index 0784f7f..fb81467 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -160,6 +160,7 @@ function TimersList(msecs, unrefed) {
 }
 
 function listOnTimeout() {
+  var pool;
   var list = this._list;
   var msecs = list.msecs;
 
@@ -222,15 +223,27 @@ function listOnTimeout() {
   // As such, we can remove the list and clean up the TimerWrap C++ handle.
   debug('%d list empty', msecs);
   assert(L.isEmpty(list));
-  this.close();
-
-  // Either refedLists[msecs] or unrefedLists[msecs] may have been removed and
-  // recreated since the reference to `list` was created. Make sure they're
-  // the same instance of the list before destroying.
-  if (list._unrefed === true && list === unrefedLists[msecs]) {
-    delete unrefedLists[msecs];
-  } else if (list === refedLists[msecs]) {
-    delete refedLists[msecs];
+
+  if (list._unrefed) {
+    pool = unrefedLists;
+  } else {
+    pool = refedLists;
+  }
+
+  /*
+   * If the timers list that was just traversed and is empty actually doesn't
+   * exist anymore (e.g because it was reused to create an unrefed timer), don't
+   * close the underlying timerwrap, its ownership changed.
+   */
+  if (pool[msecs] !== undefined) {
+    this.close();
+  }
+
+  // The timers list may have been removed and recreated since the reference to
+  // `list` was created. Make sure they're the same instance of the list before
+  // destroying.
+  if (list === pool[msecs]) {
+    delete pool[msecs];
   }
 }

?

I would think that the owner property represents ownership for a specific timer handle better than the belonging of its timer list to a given timer lists pool, so I have a preference for:

// Do not close the underlying handle if its ownership has changed
// (e.g it was unrefed in its callback).
if (this.owner)
  return;

this.close();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that if we check the closing condition weather the list was in a pool or not.

+  if (pool[msecs] !== undefined) {
+    this.close();
+  }

is better.
Otherwise, I prefer to check owner property.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very confused as to why what I originally suggested doesn't do the job just fine?

  if (list._unrefed === true && list === unrefedLists[msecs]) {
    delete unrefedLists[msecs];
    this.close();
  } else if (list === refedLists[msecs]) {
    delete refedLists[msecs];
    this.close();
  }

Keep in mind that reuse() is called before this if unrefing is actually re-using the handle and it already removed the list from the pool:

node/lib/timers.js

Lines 274 to 293 in 75cdc89

// A convenience function for re-using TimerWrap handles more easily.
//
// This mostly exists to fix https://github.com/nodejs/node/issues/1264.
// Handles in libuv take at least one `uv_run` to be registered as unreferenced.
// Re-using an existing handle allows us to skip that, so that a second `uv_run`
// will return no active handles, even when running `setTimeout(fn).unref()`.
function reuse(item) {
L.remove(item);
var list = refedLists[item._idleTimeout];
// if empty - reuse the watcher
if (list && L.isEmpty(list)) {
debug('reuse hit');
list._timer.stop();
delete refedLists[item._idleTimeout];
return list._timer;
}
return null;
}

Essentially I don't see why we'd bother to check .owner() if doing it in those conditionals is deterministic anyways?

Copy link

Choose a reason for hiding this comment

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

@Fishrock123

Essentially I don't see why we'd bother to check .owner() if doing it in those conditionals is deterministic anyways?

For the reason I mentioned above:

I would think that the owner property represents ownership for a specific timer handle better than the belonging of its timer list to a given timer lists pool

In other words, it seems it would be more robust to check the owner property than rely on what seems to be an implementation detail.

The solution you're suggesting also duplicates code, and doesn't include any documentation about why the underlying TimerWrap handle should be closed in some cases, and not in others.

What you're suggesting would indeed fix the problem, but it seems to me it would lead to a less robust solution that would be more difficult to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you're suggesting would indeed fix the problem, but it seems to me it would lead to a less robust solution that would be more difficult to understand.

I agree this.
And it is the point that we don't have an agreement with @Fishrock123 from his comment of

There are more obscure parts of the timers code imo so I don't think it is an issue.

}

// Do not close the underlying handle if its ownership has changed
// (e.g it was unrefed in its callback).
if (this.owner)
return;

this.close();
}


Expand Down
61 changes: 61 additions & 0 deletions test/parallel/test-timers-unrefed-in-callback.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
'use strict';
// Checks that setInterval timers keep running even when they're
// unrefed within their callback.

require('../common');
const assert = require('assert');
const net = require('net');

let counter1 = 0;
let counter2 = 0;

// Test1 checks that clearInterval works as expected for a timer
// unrefed within its callback: it removes the timer and its callback
// is not called anymore. Note that the only reason why this test is
// robust is that:
// 1. the repeated timer it creates has a delay of 1ms
// 2. when this test is completed, another test starts that creates a
// new repeated timer with the same delay (1ms)
// 3. because of the way timers are implemented in libuv, if two
// repeated timers A and B are created in that order with the same
// delay, it is guaranteed that the first occurrence of timer A
// will fire before the first occurrence of timer B
// 4. as a result, when the timer created by Test2 fired 11 times, if
// the timer created by Test1 hadn't been removed by clearInterval,
// it would have fired 11 more times, and the assertion in the
// process'exit event handler would fail.
function Test1() {
// server only for maintaining event loop
const server = net.createServer().listen(0);

const timer1 = setInterval(() => {
timer1.unref();
if (counter1++ === 10) {
clearInterval(timer1);
Copy link
Contributor

Choose a reason for hiding this comment

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

clearInterval works in a very specific way (in the end, it sets _idleTimeout = 0) and while it is affected in this problem it is unnecessary for testing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intended to add the clearInterval test because it executes timer.close() and to check if it is not affected by this fix. I think it can prove this fix has no side effects on timer.close() during the use of unrefed setInterval.

server.close(() => {
Test2();
});
}
}, 1);
}


// Test2 checks setInterval continues even if it is unrefed within
// timer callback. counter2 continues to be incremented more than 11
// until server close completed.
function Test2() {
// server only for maintaining event loop
const server = net.createServer().listen(0);

const timer2 = setInterval(() => {
timer2.unref();
if (counter2++ === 10)
server.close();
}, 1);
}

process.on('exit', () => {
assert.strictEqual(counter1, 11);
Copy link
Contributor

Choose a reason for hiding this comment

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

You should only need to do 3 intervals for either, regardless. That should be enough to prove it is functioning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, 10 is no meaning. Fixed to 3 intervals.

});

Test1();