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

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

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
10 participants
@abbr
Copy link

commented Aug 10, 2015

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

Fixes issue #25831.

@garrettsickles

This comment has been minimized.

Copy link

commented Aug 10, 2015

What a great fix! This should be included asap!

@leo60228

This comment has been minimized.

Copy link

commented Aug 14, 2015

+1

@misterdjules

This comment has been minimized.

Copy link

commented Aug 14, 2015

To paraphrase my comment in #25831:

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

@jasnell jasnell added the v0.12 label Aug 16, 2015

@jasnell

This comment has been minimized.

Copy link
Member

commented Aug 27, 2015

This looks good but would you be able to include a test case for it?

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

Fixes #25831.
@abbr

This comment has been minimized.

Copy link
Author

commented Aug 31, 2015

@jasnell, a test case would require either writing a c++ module or obtaining deasync via npm. I cannot find any existing cases under test/simple setup this way. In test/addons there are test cases written in c++ but that folder doesn't seem to be included in make test. Do you have any suggestion where to put the test case?

@jonathanleang

This comment has been minimized.

Copy link

commented Sep 4, 2015

👍

@jonathanleang

This comment has been minimized.

Copy link

commented Sep 4, 2015

:shipit:

@thefourtheye

This comment has been minimized.

Copy link

commented Sep 7, 2015

@vkurchatkin

This comment has been minimized.

Copy link
Member

commented Sep 7, 2015

If it's impossible to write a test without external dependencies, then it's not a bug

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Sep 7, 2015

I think this is a bug. My tired self does not think that the proposed patch could actually fix this. We'll see tomorrow maybe.

@abbr abbr force-pushed the abbr:fix-issue-timer branch from 4f2de45 to e340cb6 Sep 8, 2015

@abbr

This comment has been minimized.

Copy link
Author

commented Sep 8, 2015

I have added a test case, without any dependencies. Confirmed test case caused existing version 0.12.7 hung but passed with the fix.

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Sep 8, 2015

@abbr could we move this issue to https://github.com/nodejs/node? I'd prefer to review it there.

@vkurchatkin

This comment has been minimized.

Copy link
Member

commented Sep 8, 2015

@Fishrock123 so what's the bug? running uv_run is clearly not supported so it should be something else

@abbr

This comment has been minimized.

Copy link
Author

commented Sep 10, 2015

@Fishrock123, I have ported this pr to nodejs/node#2788

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Sep 10, 2015

Closing here.

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.