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

test: adding mustCall in test-timers-unrefed-in-callback.js #12594

Conversation

@Zahidul-Islam
Copy link

commented Apr 22, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@mscdex mscdex added the timers label Apr 22, 2017

@Fishrock123
Copy link
Member

left a comment

timer2.unref();
if (counter2++ === 3)
server.close();
}, 1);
}, 4), 1);

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 22, 2017

Contributor

I'm not sure this change is correct, especially given the comment block preceding this second test.

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Apr 22, 2017

Member

I believe it is, but it isn't what are testing so it was left out of the original.

@addaleax
Copy link
Member

left a comment

LGTM if CI is happy

@Trott

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

Changes made so running CI again.

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

@Fishrock123
Copy link
Member

left a comment

The change is now not ideal. @Zahidul-Islam Please disregard @mscdex's comment and re-add a common.mustCall() to the second timer.

@Trott

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

@Fishrock123 Doing that will result in a flaky unreliable test. That function can fire 4 times or 5 times, so we cannot use common.mustCall() with it.

Since it was not being checked in the original test, I think @mscdex is right and we should not check it here.

If we want to add a process.on('exit',...) check that it fires at least 4 times, that can be done here or it can be done in a subsequent PR. But we should not use common.mustCall(). (Or we should rewrite common.mustCall to accept an optional range, but that's definitely a separate PR.)

@Trott
Trott approved these changes Apr 23, 2017
Copy link
Member

left a comment

This LGTM as it currently stands.

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

Doing that will result in a flaky unreliable test. That function can fire 4 times or 5 times, so we cannot use common.mustCall() with it.

Please do elaborate on why that is? It will definitely fire only one or the other. If the server fully closes this loop, then 4. Otherwise, 5. Is that actually non-deterministic?

@Trott

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

@Fishrock123 The key is to run it under load. Copy this to test/parallel/test-fishrock123.js:

'use strict';
// Checks that setInterval timers keep running even when they're
// unrefed within their callback.

require('../common');
const assert = require('assert');
const net = require('net');

let counter1 = 0;
let counter2 = 0;

// Test1 checks that clearInterval works as expected for a timer
// unrefed within its callback: it removes the timer and its callback
// is not called anymore. Note that the only reason why this test is
// robust is that:
// 1. the repeated timer it creates has a delay of 1ms
// 2. when this test is completed, another test starts that creates a
//    new repeated timer with the same delay (1ms)
// 3. because of the way timers are implemented in libuv, if two
//    repeated timers A and B are created in that order with the same
//    delay, it is guaranteed that the first occurrence of timer A
//    will fire before the first occurrence of timer B
// 4. as a result, when the timer created by Test2 fired 11 times, if
//    the timer created by Test1 hadn't been removed by clearInterval,
//    it would have fired 11 more times, and the assertion in the
//    process'exit event handler would fail.
function Test1() {
  // server only for maintaining event loop
  const server = net.createServer().listen(0);

  const timer1 = setInterval(() => {
    timer1.unref();
    if (counter1++ === 3) {
      clearInterval(timer1);
      server.close(() => {
        Test2();
      });
    }
  }, 1);
}


// Test2 checks setInterval continues even if it is unrefed within
// timer callback. counter2 continues to be incremented more than 11
// until server close completed.
function Test2() {
  // server only for maintaining event loop
  const server = net.createServer().listen(0);

  const timer2 = setInterval(() => {
    timer2.unref();
    if (counter2++ === 3)
      server.close();
  }, 1);
}

process.on('exit', () => {
  assert.strictEqual(counter1, 4);
  // cause it to fail so that the value of counter2 is displayed by test.py
  assert.fail(`counter2: ${counter2}`);
});

Test1();

The only change is in the exit handler.

If I run the above on my otherwise-idle laptop, it reliably indicates that counter2 has a value of 4. But if I run it under load, I sometimes get 4 and sometimes get 5...

$ tools/test.py --repeat 128 -j 64 test/parallel/test-fishrock123.js 
=== release test-fishrock123 ===                    
Path: parallel/test-fishrock123
assert.js:86
  throw new assert.AssertionError({
  ^
AssertionError: counter2: 5
    at process.on (/Users/trott/io.js/test/parallel/test-fishrock123.js:60:10)
    at emitOne (events.js:120:20)
    at process.emit (events.js:210:7)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-fishrock123.js
=== release test-fishrock123 ===                    
Path: parallel/test-fishrock123
assert.js:86
  throw new assert.AssertionError({
  ^
AssertionError: counter2: 4
    at process.on (/Users/trott/io.js/test/parallel/test-fishrock123.js:60:10)
    at emitOne (events.js:120:20)
    at process.emit (events.js:210:7)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-fishrock123.js
=== release test-fishrock123 ===                    
Path: parallel/test-fishrock123
assert.js:86
  throw new assert.AssertionError({
  ^
AssertionError: counter2: 5
    at process.on (/Users/trott/io.js/test/parallel/test-fishrock123.js:60:10)
    at emitOne (events.js:120:20)
    at process.emit (events.js:210:7)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-fishrock123.js
=== release test-fishrock123 ===                    
Path: parallel/test-fishrock123
assert.js:86
  throw new assert.AssertionError({
  ^
AssertionError: counter2: 4
    at process.on (/Users/trott/io.js/test/parallel/test-fishrock123.js:60:10)
    at emitOne (events.js:120:20)
    at process.emit (events.js:210:7)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-fishrock123.js
=== release test-fishrock123 ===                    
Path: parallel/test-fishrock123
assert.js:86
  throw new assert.AssertionError({
  ^
AssertionError: counter2: 4
    at process.on (/Users/trott/io.js/test/parallel/test-fishrock123.js:60:10)
    at emitOne (events.js:120:20)
    at process.emit (events.js:210:7)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-fishrock123.js
=== release test-fishrock123 ===                    
Path: parallel/test-fishrock123
assert.js:86
  throw new assert.AssertionError({
...
@Zahidul-Islam

This comment has been minimized.

Copy link
Author

commented Apr 25, 2017

@Trott @Fishrock123 @addaleax @jasnell appreciate any suggestion what I should do?

@Trott

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

@Zahidul-Islam I think it's fine as-is. @Fishrock123?

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

LGTM

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 29, 2017

@Fishrock123 ping … if there are no further comments here, I am going to assume you are fine with this change as it is (especially as the diff itself here should not contain anything controversial in my eyes)

Pinged multiple times including a week ago with "if there are no further comments here, I am going to assume you are fine with this", dismissing

Trott added a commit to Trott/io.js that referenced this pull request May 6, 2017
test: add mustCall in timers-unrefed-in-callback
PR-URL: nodejs#12594
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@Trott

This comment has been minimized.

Copy link
Member

commented May 6, 2017

Landed in 3fd890a.
Thanks for the contribution! 🎉

@Trott Trott closed this May 6, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
test: add mustCall in timers-unrefed-in-callback
PR-URL: nodejs#12594
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
gibfahn added a commit that referenced this pull request Jun 20, 2017
test: add mustCall in timers-unrefed-in-callback
PR-URL: #12594
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
MylesBorins added a commit that referenced this pull request Jul 11, 2017
test: add mustCall in timers-unrefed-in-callback
PR-URL: #12594
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@MylesBorins MylesBorins referenced this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.