timer: call list.start regardless new or not #2830

abbr · 2015-09-12T06:45:49Z

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

Fixes issue nodejs/node-v0.x-archive#25831.
Ported from pr nodejs/node-v0.x-archive#25832 and #2788.

kjhangiani · 2015-09-14T03:17:38Z

+1

sterling · 2015-09-25T23:13:52Z

+1

Fishrock123 · 2015-09-26T01:00:37Z

Note: I'm on vacation and won't have time to look at this for a while.

sterling · 2015-09-26T03:28:17Z

Is there anyone who could look at it in your absence?

Fishrock123 · 2015-09-26T16:25:23Z

Cc @misterdjules / @bnoordhuis

sterling · 2015-10-12T18:07:30Z

@Fishrock123 This fix is pretty critical for me and my org. Will you be available soon to take a look at this? Thanks ahead of time.

Fishrock123 · 2015-10-12T18:10:47Z

@sterlinghw are you experiencing this due to running https://github.com/abbr/deasync, or somehow else?

Are you using uv_run in some c++ addon?

Fishrock123 · 2015-10-12T18:16:17Z

test/parallel/test-timers-active-handles.js

+    if (err) {
+      if (err === 127) {
+        console.error(
+          'node-gyp not found! Please upgrade your install of npm!'


This should follow how tests/addons are compiled/run, as this here is very brittle.

(And it needs to go into tests/addons)

follow up: running make test-addons will automatically build your addon if placed in the correct location. instructions on this are listed in my previous comment.

sterling · 2015-10-12T18:41:15Z

We are using deasync

trevnorris · 2015-10-12T19:31:50Z

test/parallel/test-timers-active-handles/deasync.cc

@@ -0,0 +1,18 @@
+#include <node.h>
+#include <uv.h>


wrong place for this test. All these files would go in test/addon/<test-name>/. If you are going to include any module then the node_module folder needs to be checked in with the test. Though we generally don't allow third party dependencies in our tests, more so in native tests. Because native APIs break faster than module authors can keep up. And we aren't in the habit of floating patches on top of other libraries.

IMO this test will need to be written stand alone.

@trevnorris The test here merely has the name of the module, there are no module deps.

Fishrock123 · 2015-10-12T20:49:20Z

@sterlinghw After further investigation, it doesn't seem like this fix works on windows at this point.

deasync relies very heavily on node's internals, and you shouldn't consider it supported by us. That is liable to break at any time. I suggest using the async workflow node was built to use.

If you really need async things to look sync, maybe take a look at co or generator-runner?

sterling · 2015-10-12T21:29:34Z

@Fishrock123 Thanks for looking into this.

As for alternatives, it's not really about making async things look sync. For our use case, it's about trying to avoid a massive refactor of our code base and deasync is the closest thing we can find that will solve the issue. There is a particular function in our code that is heavily used and integral to our application. It is currently a synchronous function, but due to new requirements, it needs to become async. This is a huge problem for us because changing it to async causes a cascade of refactors, which will ultimately cause nearly all of our sync functions to become async. For our large code base, we want to avoid this at all costs if possible.

trevnorris · 2015-10-12T21:40:32Z

test/parallel/test-timers-active-handles/deasync.cc

+using namespace v8;
+
+void Method(const FunctionCallbackInfo<Value>& args) {
+  Isolate* isolate = Isolate::GetCurrent();


Isolate* isolate = args.GetIsolate();

kjhangiani · 2015-10-12T21:50:20Z

@Fishrock123 We are in the same situation as @sterlinghw. Have already explored coroutines, generators, yield/async, various other methods, but deasync ultimately is the only library that accomplishes what we need to accomplish. Otherwise, we end up with a major refactor of pretty much every portion of our backend. Deasync is by far the cleanest way for us to satisfy our requirements, and while it depends on the internals, it's also extremely simple, and does not block node elsewhere.

We are also eagerly awaiting this fix.

trevnorris · 2015-10-12T23:25:26Z

This PR needs to be rebased on latest master. It's very out of date.

whitingj · 2015-10-12T23:53:33Z

@Fishrock123 what is the problem on Windows? Any idea of what needs to be fixed?

Call start regardless whether list is new or not to prevent incorrect active_handles count. Fixes nodejs/node-v0.x-archive#25831

abbr · 2015-10-13T01:48:09Z

@trevnorris @Fishrock123 , moved test files to addons and rebased to latest master.

vkurchatkin · 2015-10-13T02:29:22Z

As @Fishrock123 said, deasync approach is not supported. If this is a really a bug with timers, tests should contain only public API, i. e. setTimeout, clearTimeout, etc.

Fishrock123 · 2015-10-13T02:53:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/487/

Fishrock123 · 2015-10-13T04:40:43Z

@Fishrock123 what is the problem on Windows? Any idea of what needs to be fixed?

I'm pretty sure @trevnorris said that this patch errored on windows, but it is possible he was referring to something else.

Also, it turns out the CI doesn't test these on windows.

trevnorris · 2015-10-13T17:23:27Z

That comment on IRC was in regards to whether a change in this PR would fix a different issue I was facing. Sadly, it didn't.

trevnorris · 2015-10-13T17:26:16Z

test/addons/timers-active-handles/binding.cc

+#include <node.h>
+#include <uv.h>
+
+using namespace v8;


Instead we prefer using v8::Local etc. for everything this test will use.

trevnorris · 2015-10-13T17:27:50Z

Oop. I just realized you're running the event loop within the same event loop. Yeah, that's definitely not supported by libuv. Sorry.

Fishrock123 · 2015-10-13T17:44:02Z

Is anyone able to tell me when the parent issue originated? I.e. what version?

abbr · 2015-10-13T19:27:44Z

First reported in v0.12. See top of PR. But could happen long before.

On Oct 13, 2015, at 10:44 AM, Jeremiah Senkpiel notifications@github.com wrote:

Is anyone able to tell me when the parent issue originated? I.e. what version?

—
Reply to this email directly or view it on GitHub.

whitingj · 2015-10-23T20:36:40Z

@trevnorris @Fishrock123 @vkurchatkin, you guys are right that this is an unusual use case. However in the initial issue (nodejs/node-v0.x-archive#25831) filed against node js 0.12 it appears that there is a legitimate bug with how the code is handling the active handles and callback. misterdjules appears to agree this is a real bug nodejs/node-v0.x-archive#25831 (comment).

Even though the standard setTimeout and clearTimeout don't expose this problem it would be great to get this fixed. What are the chances we get this fix in?

trevnorris · 2015-10-23T21:16:11Z

Link to related discussion: #2795 (comment)

I believe part of this relies on implementation details in libuv, and platform differences on how closed timer handles are handled.

kjhangiani · 2016-03-11T21:16:49Z

Any update on this?

vkurchatkin · 2016-03-11T22:24:43Z

I'm going to close this for now, since no way to reproduce a bug using timers API was provided. We can only assume that there is no bug, just a peculiarity of timers' internals.

dy · 2016-04-08T04:20:15Z

@vkurchatkin I am not into the details of the PR, but the bug, caused the issue related with this PR still exists and quite acute. I’ve tested it over with deasync in glslify-sync in the latest node@5.10.1 and it is here.

vkurchatkin · 2016-04-08T11:47:04Z

@dfcreative it doesn't seems to be a bug in node

dy · 2016-04-08T14:19:04Z

@vkurchatkin maybe it is not, it was just marked as a bug for 0.12 and 4.x, 5.x reproduce it. Anyways it is preventing a bunch of really useful packages to work as they could.

mermaid · 2016-04-08T14:29:10Z

I also agree with @dfcreative. It's definitely a bug to me. Maybe it's a bug that can only exposed using native extensions, but its still a bug thats causing problems for me.

jasnell · 2016-04-08T15:37:26Z

Reopening so this can be investigated further.

vkurchatkin · 2016-04-08T15:39:05Z

There is no way to reproduce it using public APIs, so it's not bug. What deasync does is not supported

jasnell · 2016-04-08T15:52:42Z

I get that, I just want to look into this a bit to see if there's something else that can be done here.

Fishrock123 · 2016-04-08T15:53:11Z

I have unmarked it as a bug in the old repo. Perhaps this is solvable, but using deasync is quite literally UNSAFE.

Fishrock123 · 2016-04-08T15:55:02Z

lib/timers.js


    L.init(list);

    lists[msecs] = list;
    list.msecs = msecs;
    list[kOnTimeout] = listOnTimeout;
  }
+  // Call start regardless whether list is new
+  // or not to prevent incorrect active_handles
+  // count. See https://github.com/nodejs/node-v0.x-archive/issues/25831.


This new logic is definitely not correct.

If you do this you are at very high risk (almost 100%) of delaying existing, running timers. Read my extensive comments in

node/lib/timers.js

Lines 68 to 78 in 7c9a691

// With this, virtually constant-time insertion (append), removal, and timeout

// is possible in the JavaScript layer. Any one list of timers is able to be

// sorted by just appending to it because all timers within share the same

// duration. Therefore, any timer added later will always have been scheduled to

// timeout later, thus only needing to be appended.

// Removal from an object-property linked list is also virtually constant-time

// as can be seen in the lib/internal/linkedlist.js implementation.

// Timeouts only need to process any timers due to currently timeout, which will

// always be at the beginning of the list for reasons stated above. Any timers

// after the first one encountered that does not yet need to timeout will also

// always be due to timeout at a later time.

on how this works if you don't believe me.

Perhaps it would work correctly if you did the following:

list.start(Timer.now() - list._idlePrev._idleStart, 0);

If that worked, I would be willing to entertain changing the impl to that.

(Note: made comment on a different line so it would show at the bottom of the issue thread.)

jasnell · 2016-04-08T16:03:47Z

Oh yeah, I'm definitely not advocating this particular proposed approach... just that I would like to investigate the kind of alternatives you're suggesting. It likely will turn out exactly as @vkurchatkin suggests (that we simply close again without further action) but I'd like to at least explore a bit more :-)

jasnell · 2016-04-19T05:47:12Z

Spent some time going through this over this weekend and pretty much reached the same conclusion.

brendanashworth added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Sep 12, 2015

Fishrock123 self-assigned this Sep 13, 2015

Fishrock123 reviewed Oct 12, 2015
View reviewed changes

trevnorris reviewed Oct 12, 2015
View reviewed changes

timer: call list.start regardless new or not

966b336

Call start regardless whether list is new or not to prevent incorrect active_handles count. Fixes nodejs/node-v0.x-archive#25831

abbr force-pushed the fix-issue-timer-master branch from 6e6d304 to 966b336 Compare October 13, 2015 01:43

abbr mentioned this pull request Oct 13, 2015

deasync hangs in an asynchronous context abbr/deasync#21

Open

Fishrock123 mentioned this pull request Oct 13, 2015

Test addons on windows test-ci #3339

Closed

trevnorris reviewed Oct 13, 2015
View reviewed changes

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:00

vkurchatkin closed this Mar 11, 2016

jasnell reopened this Apr 8, 2016

Fishrock123 reviewed Apr 8, 2016
View reviewed changes

jasnell added the wip Issues and PRs that are still a work in progress. label Apr 8, 2016

jasnell closed this Apr 19, 2016

dead-claudia mentioned this pull request Oct 26, 2016

Lint multiple files in parallel [$500] eslint/eslint#3565

Open

mcjc29 mentioned this pull request Jul 26, 2018

On updating to version 2 tests are not running - possible issue with deasync pact-foundation/karma-pact#15

Closed

	// With this, virtually constant-time insertion (append), removal, and timeout
	// is possible in the JavaScript layer. Any one list of timers is able to be
	// sorted by just appending to it because all timers within share the same
	// duration. Therefore, any timer added later will always have been scheduled to
	// timeout later, thus only needing to be appended.
	// Removal from an object-property linked list is also virtually constant-time
	// as can be seen in the lib/internal/linkedlist.js implementation.
	// Timeouts only need to process any timers due to currently timeout, which will
	// always be at the beginning of the list for reasons stated above. Any timers
	// after the first one encountered that does not yet need to timeout will also
	// always be due to timeout at a later time.

timer: call list.start regardless new or not #2830

timer: call list.start regardless new or not #2830

Conversation

abbr commented Sep 12, 2015

kjhangiani commented Sep 14, 2015

sterling commented Sep 25, 2015

Fishrock123 commented Sep 26, 2015

sterling commented Sep 26, 2015

Fishrock123 commented Sep 26, 2015

sterling commented Oct 12, 2015

Fishrock123 commented Oct 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sterling commented Oct 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Oct 12, 2015

sterling commented Oct 12, 2015

Choose a reason for hiding this comment

kjhangiani commented Oct 12, 2015

trevnorris commented Oct 12, 2015

whitingj commented Oct 12, 2015

abbr commented Oct 13, 2015

vkurchatkin commented Oct 13, 2015

Fishrock123 commented Oct 13, 2015

Fishrock123 commented Oct 13, 2015

trevnorris commented Oct 13, 2015

Choose a reason for hiding this comment

trevnorris commented Oct 13, 2015

Fishrock123 commented Oct 13, 2015

abbr commented Oct 13, 2015

whitingj commented Oct 23, 2015

trevnorris commented Oct 23, 2015

kjhangiani commented Mar 11, 2016

vkurchatkin commented Mar 11, 2016

dy commented Apr 8, 2016

vkurchatkin commented Apr 8, 2016

dy commented Apr 8, 2016

mermaid commented Apr 8, 2016

jasnell commented Apr 8, 2016

vkurchatkin commented Apr 8, 2016

jasnell commented Apr 8, 2016

Fishrock123 commented Apr 8, 2016

Choose a reason for hiding this comment

jasnell commented Apr 8, 2016

jasnell commented Apr 19, 2016