Skip to content
This repository has been archived by the owner. It is now read-only.

Incorrect loop->active_handles count when nested setTimeout calls have same value #25831

Open
abbr opened this Issue Aug 10, 2015 · 3 comments

Comments

Projects
None yet
5 participants
@abbr
Copy link

abbr commented Aug 10, 2015

I maintain a c++ module deasync (originally written by @vkurchatkin) which essentially exposes uv_run to jsLand in order to address some use cases that cannot be handled by other async-2-sync modules. A problem was reported recently that when deasync is involved in nested setTimeout calls with same timeout value, the process hung. It is illustrated by code

var deasync = require('deasync');

function async(cb) {
  console.log('x');
  setTimeout(function () {
    console.log('y');
    cb(null, 'value');
    console.log('z');
  }, 8);
}

setTimeout(function () {
  console.log('A', deasync(async)());
}, 8); // changing 8 to, say 9, works

output

x
(hung)

Changing one of the timeout values of setTimeout yields expected output

x
y
z
A value

Problem is reproducible in latest Node version on at least Linux and Windows. I have run node in gdb session and found the problem is loop->active_handles has gone down to 0 after the inner setTimeout is called but before its handler is triggered, therefore the inner handler doesn't have a chance to run.

I think the cause is that timers of same timeout value are implemented using a linked list and only one handle is exposed to libuv. This handle is removed at the time the outer setTimeout handler is called.

By moving list.start(msecs, 0) in /lib/timers.js outside else block, I have verified the problem can be fixed. Will supply a PR later. What I am not clear is the impact of this change. Appreciate if someone can review the issue and my PR.

(P.S. I have read Node issue reporting guidelines and understand I am not supposed include dependencies. But deasync is simple, has no dependency by itself, and is easy to be re-factored into a dependency-less mixture of c++ and js code)

abbr added a commit to abbr/node that referenced this issue Aug 10, 2015

timer: call list.start regardless new or not
Call start regardless whether list is new
or not to prevent incorrect active_handles
count.

Fixes nodejs#25831.
@tunniclm

This comment has been minimized.

Copy link

tunniclm commented Aug 14, 2015

Could be related to #25763

@misterdjules

This comment has been minimized.

Copy link

misterdjules commented Aug 14, 2015

@tunniclm Although it seems at first sight, this issue is not related to #25763 :) @abbr's analysis is spot on.

While deasync seems to rely too much on Node.js' internals, the fix suggested in #25832 actually looks good to me so far, and thus I think I would be inclined to merge it in.

Thank you both @abbr @tunniclm for your input!

@misterdjules misterdjules added this to the 0.12.8 milestone Aug 14, 2015

abbr added a commit to abbr/node that referenced this issue Sep 8, 2015

timer: call list.start regardless new or not
Call start regardless whether list is new
or not to prevent incorrect active_handles
count.

Fixes nodejs#25831.

abbr added a commit to abbr/node-1 that referenced this issue Sep 10, 2015

timer: call list.start regardless new or not
Call start regardless whether list is new
or not to prevent incorrect active_handles
count.

Fixes nodejs/node-v0.x-archive#25831.

abbr added a commit to abbr/node-1 that referenced this issue Oct 13, 2015

timer: call list.start regardless new or not
Call start regardless whether list is new
or not to prevent incorrect active_handles
count.

Fixes nodejs/node-v0.x-archive#25831
@kjhangiani

This comment has been minimized.

Copy link

kjhangiani commented Mar 11, 2016

Has there been any progress on this? Does this fix have other potential consequences?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.