-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: add test-domain-exit-dispose-again back #4256
Closed
misterdjules
wants to merge
2
commits into
nodejs:master
from
misterdjules:readd-test-domain-exit-dispose-again
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,39 +1,76 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const domain = require('domain'); | ||
|
||
// Use the same timeout value so that both timers' callbacks are called during | ||
// the same invocation of the underlying native timer's callback (listOnTimeout | ||
// in lib/timers.js). | ||
setTimeout(err, 50); | ||
setTimeout(common.mustCall(secondTimer), 50); | ||
// This test makes sure that when a domain is disposed, timers that are | ||
// attached to that domain are not fired, but timers that are _not_ attached | ||
// to that domain, including those whose callbacks are called from within | ||
// the same invocation of listOnTimeout, _are_ called. | ||
|
||
function err() { | ||
var common = require('../common'); | ||
var assert = require('assert'); | ||
var domain = require('domain'); | ||
var disposalFailed = false; | ||
|
||
// Repeatedly schedule a timer with a delay different than the timers attached | ||
// to a domain that will eventually be disposed to make sure that they are | ||
// called, regardless of what happens with those timers attached to domains | ||
// that will eventually be disposed. | ||
var a = 0; | ||
log(); | ||
function log() { | ||
console.log(a++, process.domain); | ||
if (a < 10) setTimeout(log, 20); | ||
} | ||
|
||
var secondTimerRan = false; | ||
|
||
// Use the same timeout duration for both "firstTimer" and "secondTimer" | ||
// callbacks so that they are called during the same invocation of the | ||
// underlying native timer's callback (listOnTimeout in lib/timers.js). | ||
const TIMEOUT_DURATION = 50; | ||
|
||
setTimeout(function firstTimer() { | ||
const d = domain.create(); | ||
d.on('error', handleDomainError); | ||
d.run(err2); | ||
|
||
function err2() { | ||
// this function doesn't exist, and throws an error as a result. | ||
d.on('error', function handleError(err) { | ||
// Dispose the domain on purpose, so that we can test that nestedTimer | ||
// is not called since it's associated to this domain and a timer whose | ||
// domain is diposed should not run. | ||
d.dispose(); | ||
console.error(err); | ||
console.error('in domain error handler', | ||
process.domain, process.domain === d); | ||
}); | ||
|
||
d.run(function() { | ||
// Create another nested timer that is by definition associated to the | ||
// domain "d". Because an error is thrown before the timer's callback | ||
// is called, and because the domain's error handler disposes the domain, | ||
// this timer's callback should never run. | ||
setTimeout(function nestedTimer() { | ||
console.error('Nested timer should not run, because it is attached to ' + | ||
'a domain that should be disposed.'); | ||
disposalFailed = true; | ||
process.exit(1); | ||
}); | ||
|
||
// Make V8 throw an unreferenced error. As a result, the domain's error | ||
// handler is called, which disposes the domain "d" and should prevent the | ||
// nested timer that is attached to it from running. | ||
err3(); | ||
} | ||
}); | ||
}, TIMEOUT_DURATION); | ||
|
||
function handleDomainError(e) { | ||
// In the domain's error handler, the current active domain should be the | ||
// domain within which the error was thrown. | ||
assert.equal(process.domain, d); | ||
} | ||
} | ||
// This timer expires in the same invocation of listOnTimeout than firstTimer, | ||
// but because it's not attached to any domain, it must run regardless of | ||
// domain "d" being disposed. | ||
setTimeout(function secondTimer() { | ||
console.log('In second timer'); | ||
secondTimerRan = true; | ||
}, TIMEOUT_DURATION); | ||
|
||
function secondTimer() { | ||
// secondTimer was scheduled before any domain had been created, so its | ||
// callback should not have any active domain set when it runs. | ||
// Do not use assert here, as it throws errors and if a domain with an error | ||
// handler is active, then asserting wouldn't make the test fail. | ||
if (process.domain !== null) { | ||
console.log('process.domain should be null, but instead is:', | ||
process.domain); | ||
process.exit(1); | ||
} | ||
} | ||
process.on('exit', function() { | ||
assert.equal(a, 10); | ||
assert.equal(disposalFailed, false); | ||
assert(secondTimerRan); | ||
console.log('ok'); | ||
}); |
45 changes: 45 additions & 0 deletions
45
test/parallel/test-timers-reset-process-domain-on-throw.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
'use strict'; | ||
|
||
// This test makes sure that when throwing from within a timer's callback, | ||
// its active domain at the time of the throw is not the process' active domain | ||
// for the next timers that need to be processed on the same turn of the event | ||
// loop. | ||
|
||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const domain = require('domain'); | ||
|
||
// Use the same timeout value so that both timers' callbacks are called during | ||
// the same invocation of the underlying native timer's callback (listOnTimeout | ||
// in lib/timers.js). | ||
setTimeout(err, 50); | ||
setTimeout(common.mustCall(secondTimer), 50); | ||
|
||
function err() { | ||
const d = domain.create(); | ||
d.on('error', handleDomainError); | ||
d.run(err2); | ||
|
||
function err2() { | ||
// this function doesn't exist, and throws an error as a result. | ||
err3(); | ||
} | ||
|
||
function handleDomainError(e) { | ||
// In the domain's error handler, the current active domain should be the | ||
// domain within which the error was thrown. | ||
assert.equal(process.domain, d); | ||
} | ||
} | ||
|
||
function secondTimer() { | ||
// secondTimer was scheduled before any domain had been created, so its | ||
// callback should not have any active domain set when it runs. | ||
if (process.domain !== null) { | ||
console.log('process.domain should be null in this timer callback, but ' + | ||
'instead is:', process.domain); | ||
// Do not use assert here, as it throws errors and if a domain with an error | ||
// handler is active, then asserting wouldn't make the test fail. | ||
process.exit(1); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we avoid using
process.exit()
in tests because it is rather immediate, is there a way this could be switched to Throw?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is explained by the comment above:
maybe this comment should be moved right above the call to
process.exit(1)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, probably. And of course, domains, oopsie.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll move it closer to the call to
process.exit
.