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

fix: do not exit when only unref'd timer is present in test code #3825

Merged
merged 8 commits into from
Jul 20, 2024

Conversation

boneskull
Copy link
Contributor

@boneskull boneskull commented Mar 11, 2019

In the timeout-handling code in Runnable#resetTimeout, if _enableTimeouts was false, Mocha would completely avoid setting a timer, even if the timeout was set to 0.

This PR changes the behavior so that a timer is always set, regardless of _enableTimeouts. If the test timeout value is 0, we set it to the maximum delay value (docs). Even if you happen to leave Mocha running in your debugger for longer than 24.8 days, the timer will just reset itself instead of throw a timeout error. Talk about corner cases...

UPDATE: Resetting the timeout is problematic if timeouts are disabled after the timeout is reset. We don't really need this behavior anyway.

Fixes #3817.

@boneskull boneskull added type: bug a defect, confirmed by a maintainer semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Mar 11, 2019
@boneskull boneskull added the area: node.js command-line-or-Node.js-specific label Mar 11, 2019
@boneskull
Copy link
Contributor Author

@rolfl, can you give this code a try?

@boneskull
Copy link
Contributor Author

I note that _enableTimeouts is essentially a useless, internal flag that could be removed (across several different files) if we assume a timeout value of 0 is false and anything else is true.

@rolfl
Copy link

rolfl commented Mar 11, 2019

@boneskull - I pulled your branch in to my local copy of mocha, I then npm link'd it in to my own project's npm repo, and ran the tests that initially caused my failues, in both normal, and debug mode, using webstorm to do it. (i.e. the worst case scenario). I also ran individual tests as selected from the WebStorm UI that uses the --grep functionality to run individual tests only.

I can (happily) report a success. It ran all tests and skipped none (and these things were failing before).

@juergba
Copy link
Contributor

juergba commented Mar 11, 2019

How/when is the timeout cleared?

@rolfl
Copy link

rolfl commented Mar 11, 2019

@juergba - when the task completes:

self.clearTimeout();

@boneskull
Copy link
Contributor Author

Unfortunately there's more to be done here. My guess is there's a problem with --no-exit / --exit behavior, as the builds seem to hang.

@rolfl
Copy link

rolfl commented Mar 11, 2019

@boneskull - I ran in to the unit tests here hanging with --timeout 0 - as I mentioned in #3817. It may help you narrow down the issue... maybe. https://github.com/mochajs/mocha/blob/master/test/unit/mocha.spec.js#L281

@boneskull
Copy link
Contributor Author

what I'm seeing is a hang in the Runnable unit tests around some edge-case stuff

…3817

Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@boneskull
Copy link
Contributor Author

alright, think I fixed that problem.

@coveralls
Copy link

coveralls commented Mar 11, 2019

Coverage Status

coverage: 94.33% (-0.03%) from 94.359%
when pulling 1dbfcf7 on boneskull/issue/3817
into 2f3fedc on master.

@boneskull
Copy link
Contributor Author

I'm going to sit on this until I get at least one review

@juergba
Copy link
Contributor

juergba commented Mar 12, 2019

out of topic: when debugging with VSCode, the CL input looks like this:
node.exe --inspect-brk=19942 bin\_mocha D:/mocha3291/unref.js

This way the --inspect-brk is consumed directly by node and is not piped through Mocha first. Therefore timeout=0 has to be set manually while debugging in order to avoid a timeout error.

Nevertheless docu states:

Implies --no-timeout.

@juergba
Copy link
Contributor

juergba commented Mar 12, 2019

When I run

'use strict';

it('just kind of stops', function(done) {
  setTimeout(done, 10).unref();
});

it('just kind of stops', function(done) {
    setTimeout(done, 10).unref();
});

the output is:

√ just kind of stops
√ just kind of stops

2 passing (31ms)

while debugging the same code, the output contains additional duration information:

√ just kind of stops (9410ms)
√ just kind of stops (5798ms)
2 passing (15s)

@juergba
Copy link
Contributor

juergba commented Mar 12, 2019

This solution sets a timer for each individual test. Just as if user would call --timeout 999999 by himself or Mocha translates --timeout 0 into --timeout 999999 for convenience.

I would prefer a root timer, similar to the one proposed by @rolfl.

In "Mocha.prototype.run":

var runner = new exports.Runner(suite, options.delay);
  var setTimeout = global.setTimeout;
  if (suite._timeout === 0) {
    runner.rootTimer = setTimeout(function() {}, 999999);
  }

rootTimer has to be cleared on the EVENT_RUN_END event.
This works and "npm test" terminates successfully. Yes, there is this issue with the global timers.

@boneskull
Copy link
Contributor Author

boneskull commented Mar 12, 2019

I would prefer a root timer, similar to the one proposed by @rolfl.

I'd have to see this working to be convinced it's the right choice.

UPDATE:

Let me clarify.

  1. This of course adds an extra timer, regardless of the value of timeout.
  2. Most runs of Mocha will have a nonzero timeout value.
  3. I don't fully understand the impact and if there are any unintended consequences to doing this (FUD). Does this impact watch mode, signals, --exit/--no-exit, browsers? While our test coverage is pretty OK in Node.js, there's always edge cases and interplay between various usages that we're missing.

This PR sticks to the "status quo", so I'm more comfortable with this strategy.

@boneskull
Copy link
Contributor Author

√ just kind of stops (9410ms)
√ just kind of stops (5798ms)
2 passing (15s)

This is "slow" output. People using --timeout 0 or --timeout 99999999 don't usually bother setting --slow 0 or --slow 99999999999 anyway; this is how you'd suppress it.

I wouldn't be opposed to updating the default value of slow to be a function of the value of timeout (I've mentioned this before) but it doesn't belong in this PR.

@boneskull
Copy link
Contributor Author

This way the --inspect-brk is consumed directly by node and is not piped through Mocha first. Therefore timeout=0 has to be set manually while debugging in order to avoid a timeout error.

This is why the debuggers which, when creating a "Mocha" debug profile, use node bin/_mocha --inspect-brk --timeout <value>.

The documentation is technically correct, but it might be worth mentioning that this only applies when executing bin/mocha. Please create an issue?

@rolfl
Copy link

rolfl commented Mar 12, 2019

@boneskull - re:

I would prefer a root timer, similar to the one proposed by @rolfl.

I'd have to see this working to be convinced it's the right choice.

In all my testing with the solution I proposed (using an interval instead of a timeout to avoid the 24-day limit) it works really well except for in the base mocha.#run() test case, where it appears sinon is interfering with either the setInterval or the clearInterval. I can investigate further, but the basic logic is similar to the logic you propose, except I put it out her for the overall run, wheras your change above puts it there for the individual suites. I put a ref() interval on the event loop that prevents node exit... and clear it when the mocha.run completes (on end event or done(...) call.

@boneskull
Copy link
Contributor Author

@rolfl I can't see what you did, but you can't just use setTimeout or setInterval without dereferencing it, e.g. var setInterval = global.setInterval, because Sinon (or Unexpected, or any other test helper) could monkey with it. you may have gotten an ESLint error if this was the case.

@boneskull
Copy link
Contributor Author

@juergba Would like to know if you are insistent on a "global" timer?

juergba
juergba previously approved these changes Mar 14, 2019
Copy link
Contributor

@juergba juergba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juergba Would like to know if you are insistent on a "global" timer?

No, I don't insist.

I note that _enableTimeouts is essentially a useless, internal flag that could be removed (across several different files) if we assume a timeout value of 0 is false and anything else is true.

As I understood we need now both properties. In our unref() case here, timeout is set to MAX_TIMEOUT and _enableTimeouts remains false.

@juergba
Copy link
Contributor

juergba commented Mar 14, 2019

BTW just noted, there is a very old branch about that issue: ead3576

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: node.js command-line-or-Node.js-specific semver-patch implementation requires increase of "patch" version number; "bug fixes" status: in triage a maintainer should (re-)triage (review) this issue type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug: --timeout 0 with unref'd timers causes process exit
6 participants