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 #2788

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@abbr
Copy link

abbr commented Sep 10, 2015

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

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

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.
@@ -0,0 +1,36 @@
var cp = require('child_process');

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Sep 10, 2015

Contributor

use strict should be the first line.

@@ -0,0 +1,36 @@
var cp = require('child_process');
var assert = require('assert');

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Sep 10, 2015

Contributor

use const wherever you assign the value once and never reassign anything to the same.


// build deasync
cp.spawn(
process.platform === 'win32' ? 'node-gyp.cmd' : 'node-gyp', ['rebuild'], {

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Sep 10, 2015

Contributor

require common at the top and use common.isWindows

@thefourtheye

This comment has been minimized.

Copy link
Contributor

thefourtheye commented Sep 10, 2015

The simple directory under test directory is specific to the old repository. Here you need to put this either in parallel or sequential directory.

@mscdex

This comment has been minimized.

Copy link
Contributor

mscdex commented Sep 10, 2015

Is it possible to create a test that doesn't have to compile C++ code (while tests are running)? Maybe utilize process._getActiveHandles() to check the active handle count?

@abbr

This comment has been minimized.

Copy link
Author

abbr commented Sep 10, 2015

@mscdex, the c++ code is used to expose uv_run to js, not for reporting active handles.

@vkurchatkin

This comment has been minimized.

Copy link
Member

vkurchatkin commented Sep 10, 2015

Why do you need uv_run to test incorrect active handles count?

@Fishrock123

This comment has been minimized.

Copy link
Member

Fishrock123 commented Sep 10, 2015

Please move the tests into test/parallel/ :)

@abbr

This comment has been minimized.

Copy link
Author

abbr commented Sep 12, 2015

Just realized pr should be applied to master branch. I will close this pr and submit a new one.

@mscdex, in my updated test case of the new pr, I found process._getActiveHandles().length is reporting wrong # of count: 3, as opposed to c++ uv_default_loop()->active_handles of 0 which is used in the test case. Possibly another bug.

@vkurchatkin , uv_run is not used to test incorrect handles. It's how the problem occurs. Just nesting two setTimeout calls with same value won't cause the problem.

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.