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.

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

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

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+42 −1
Diff settings

Always

Just for now

@@ -40,14 +40,17 @@ exports.active = function(item) {
list = lists[msecs];
} else {
list = new Timer();
list.start(msecs, 0);

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 comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Apr 8, 2016

Member

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

list.start(msecs, 0);

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Oct 12, 2015

Member

What happens if the previous list start time was less than this list start time? I think it should check that, if I am following the logic correctly.

This comment has been minimized.

Copy link
@Fishrock123

L.append(list, item);
assert(!L.isEmpty(list)); // list is not empty
@@ -0,0 +1,17 @@
#include <node.h>
#include <uv.h>

using namespace v8;

This comment has been minimized.

Copy link
@trevnorris

trevnorris Oct 13, 2015

Contributor

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


void Method(const FunctionCallbackInfo<Value>& args) {
Isolate* isolate = args.GetIsolate();
int hCnt = uv_default_loop()->active_handles;
uv_run(uv_default_loop(), UV_RUN_ONCE);
args.GetReturnValue().Set(Integer::New(isolate, hCnt));
}

void init(Handle<Object> exports) {
NODE_SET_METHOD(exports, "run", Method);
}

NODE_MODULE(deasync, init)
@@ -0,0 +1,8 @@
{
'targets': [
{
'target_name': 'binding',
'sources': [ 'binding.cc' ]
}
]
}
@@ -0,0 +1,13 @@
'use strict';

const assert = require('assert');
var uvRunOnce = require('./build/Release/binding');
setTimeout(function () {
var res;
setTimeout(function () {
res = true;
}, 2);
while (!res && uvRunOnce.run()) {
}
assert.equal(res, true);
}, 2);
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.