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
base: master
from

Conversation

Projects
None yet
@abbr
Copy link

abbr commented Sep 12, 2015

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.

@Fishrock123 Fishrock123 self-assigned this Sep 13, 2015

@kjhangiani

This comment has been minimized.

Copy link

kjhangiani commented Sep 14, 2015

+1

1 similar comment
@sterling

This comment has been minimized.

Copy link

sterling commented Sep 25, 2015

+1

@Fishrock123

This comment has been minimized.

Copy link
Member

Fishrock123 commented Sep 26, 2015

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

@sterling

This comment has been minimized.

Copy link

sterling commented Sep 26, 2015

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

@Fishrock123

This comment has been minimized.

Copy link
Member

Fishrock123 commented Sep 26, 2015

@sterling

This comment has been minimized.

Copy link

sterling commented Oct 12, 2015

@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

This comment has been minimized.

Copy link
Member

Fishrock123 commented Oct 12, 2015

@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

View changes

test/parallel/test-timers-active-handles.js Outdated
if (err) {
if (err === 127) {
console.error(
'node-gyp not found! Please upgrade your install of npm!'

This comment has been minimized.

@Fishrock123

Fishrock123 Oct 12, 2015

Member

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

(And it needs to go into tests/addons)

This comment has been minimized.

@trevnorris

trevnorris Oct 12, 2015

Contributor

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.

// 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.
list.start(msecs, 0);

This comment has been minimized.

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

@Fishrock123
@sterling

This comment has been minimized.

Copy link

sterling commented Oct 12, 2015

We are using deasync

@trevnorris

View changes

test/parallel/test-timers-active-handles/deasync.cc Outdated
@@ -0,0 +1,18 @@
#include <node.h>
#include <uv.h>

This comment has been minimized.

@trevnorris

trevnorris Oct 12, 2015

Contributor

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.

This comment has been minimized.

@Fishrock123

Fishrock123 Oct 12, 2015

Member

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

This comment has been minimized.

@trevnorris

trevnorris Oct 12, 2015

Contributor

My bad.

@Fishrock123

This comment has been minimized.

Copy link
Member

Fishrock123 commented Oct 12, 2015

@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

This comment has been minimized.

Copy link

sterling commented Oct 12, 2015

@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

View changes

test/parallel/test-timers-active-handles/deasync.cc Outdated
using namespace v8;

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

This comment has been minimized.

@trevnorris

trevnorris Oct 12, 2015

Contributor

Isolate* isolate = args.GetIsolate();

@trevnorris

View changes

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

void Method(const FunctionCallbackInfo<Value>& args) {
Isolate* isolate = Isolate::GetCurrent();
HandleScope scope(isolate);

This comment has been minimized.

@trevnorris

trevnorris Oct 12, 2015

Contributor

No need for HandleScope on calls made from JS.

@kjhangiani

This comment has been minimized.

Copy link

kjhangiani commented Oct 12, 2015

@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

This comment has been minimized.

Copy link
Contributor

trevnorris commented Oct 12, 2015

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

@whitingj

This comment has been minimized.

Copy link

whitingj commented Oct 12, 2015

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

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 abbr force-pushed the abbr:fix-issue-timer-master branch to 966b336 Oct 13, 2015

@abbr

This comment has been minimized.

Copy link
Author

abbr commented Oct 13, 2015

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

@vkurchatkin

This comment has been minimized.

Copy link
Member

vkurchatkin commented Oct 13, 2015

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

This comment has been minimized.

@Fishrock123

This comment has been minimized.

Copy link
Member

Fishrock123 commented Oct 13, 2015

@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

This comment has been minimized.

Copy link
Contributor

trevnorris commented Oct 13, 2015

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.

#include <node.h>
#include <uv.h>

using namespace v8;

This comment has been minimized.

@trevnorris

trevnorris Oct 13, 2015

Contributor

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

@trevnorris

This comment has been minimized.

Copy link
Contributor

trevnorris commented Oct 13, 2015

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

This comment has been minimized.

Copy link
Member

Fishrock123 commented Oct 13, 2015

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

@abbr

This comment has been minimized.

Copy link
Author

abbr commented Oct 13, 2015

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

This comment has been minimized.

Copy link

whitingj commented Oct 23, 2015

@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

This comment has been minimized.

Copy link
Contributor

trevnorris commented Oct 23, 2015

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.

@Trott Trott force-pushed the nodejs:master branch to 082cc8d Dec 27, 2015

@kjhangiani

This comment has been minimized.

Copy link

kjhangiani commented Mar 11, 2016

Any update on this?

@vkurchatkin

This comment has been minimized.

Copy link
Member

vkurchatkin commented Mar 11, 2016

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

This comment has been minimized.

Copy link

dy commented Apr 8, 2016

@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

This comment has been minimized.

Copy link
Member

vkurchatkin commented Apr 8, 2016

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

@dy

This comment has been minimized.

Copy link

dy commented Apr 8, 2016

@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

This comment has been minimized.

Copy link

mermaid commented Apr 8, 2016

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

This comment has been minimized.

Copy link
Member

jasnell commented Apr 8, 2016

Reopening so this can be investigated further.

@jasnell jasnell reopened this Apr 8, 2016

@vkurchatkin

This comment has been minimized.

Copy link
Member

vkurchatkin commented Apr 8, 2016

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

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Apr 8, 2016

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

This comment has been minimized.

Copy link
Member

Fishrock123 commented Apr 8, 2016

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


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.

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

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Apr 8, 2016

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

This comment has been minimized.

Copy link
Member

jasnell commented Apr 19, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.