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

timers: refactor setImmediate error handling #17879

Closed

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Dec 27, 2017

If an error is encountered during the processing of Immediates, schedule the remaining queue to finish after all error handling code runs (if the process is still alive to do so). The new changes make the Immediates error handling behaviour entirely deterministic and predictable, as the full queue will be flushed on each Immediates cycle, regardless of whether an error is encountered or not.

Currently this processing is scheduled for nextTick which can yield unpredictable results as the nextTick might happen as early as close callbacks phase or as late as after the next event loop turns Immediates all fully process. The latter can result in two full cycles of Immediates processing during one event loop turn.

The current implementation also doesn't differentiate between Immediates scheduled for the current queue run or the next one, so Immediates that were scheduled for the next turn of the event loop, will process alongside the ones that were scheduled for the current turn.

It's possible this behaviour is semver-minor/semver-major although I would argue it's at worst semver-patch, as the current behaviour is entirely unpredictable and highly depends on what other operations are scheduled after the error handling is resolved.

CI: https://ci.nodejs.org/job/node-test-pull-request/12314/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1174/

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

src, timers, test

@apapirovski apapirovski added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Dec 27, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 27, 2017
src/env.h Outdated
kFieldsCount
};

uint32_t fields_[kFieldsCount];
Copy link
Member

Choose a reason for hiding this comment

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

From an initial look: It’s your call whether turning this into a class is helpful or not, but in any case the storage should only be accessed as an AliasedBuffer, since that is necessary for ChakraCore’s time-travel debugging to work with these values (because otherwise it can’t witness the changes from C++).

If you miss the availability of ++ and --, I guess it’s fair to add those to the AliasedBuffer reference class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, wasn't aware of that. Will make some changes.

lib/timers.js Outdated
var immediateQueue = new ImmediateList();
const immediateQueue = new ImmediateList();

// If an error is encountered during execution of immediateQueue,
Copy link
Member

Choose a reason for hiding this comment

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

Optional: I would be specific and say “If an uncaught exception was thrown during execution …”

lib/timers.js Outdated
if (!immediate._onImmediate) {
immediate = immediate._idleNext;
continue;
}

if (domain)
domain.enter();
Copy link
Member

Choose a reason for hiding this comment

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

Just making sure: These are a drive-by fix and not related to the exception handling itself, because domains use async hooks now anyway. Right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm actually thinking that this should be a separate PR because this change could be back-ported but the domain portion probably can't.

lib/timers.js Outdated
const [activateImmediateCheck, scheduledImmediateCountArray] =
// should match ImmediateInfo in src/env.h
const kCount = 0;
const kOutstanding = 1;
Copy link
Member

Choose a reason for hiding this comment

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

It’s not really obvious that this is a 0/1 flag, so I’d prefer to have it called something like kHasOutstanding?

src/env-inl.h Outdated
return fields_[kCount];
}

inline uint32_t Environment::ImmediateInfo::outstanding() const {
Copy link
Member

Choose a reason for hiding this comment

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

This could return a boolean too, I guess?

@apapirovski apapirovski force-pushed the patch-timers-immediate-error branch 2 times, most recently from 0c72b4f to 52d65d8 Compare December 27, 2017 19:05
@apapirovski apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017
If an error is encountered during the processing of Immediates, schedule
the remaining queue to finish after all error handling code runs (if the
process is still alive to do so). The new changes make the Immediates
error handling behaviour entirely deterministic and predictable, as the
full queue will be flushed on each Immediates cycle, regardless of
whether an error is encountered or not.

Currently this processing is scheduled for nextTick which can yield
unpredictable results as the nextTick might happen as early as close
callbacks phase or as late as after the next event loop turns Immediates
all fully processed. The latter can result in two full cycles of
Immediates processing during one even loop turn.

The current implementation also doesn't differentiate between Immediates
scheduled for the current queue run or the next one, so Immediates that
were scheduled for the next turn of the event loop, will process
alongside the ones that were scheduled for the current turn.
@apapirovski
Copy link
Member Author

@apapirovski
Copy link
Member Author

Landed in 54062d3

@apapirovski apapirovski closed this Jan 2, 2018
@apapirovski apapirovski deleted the patch-timers-immediate-error branch January 2, 2018 16:17
apapirovski added a commit that referenced this pull request Jan 2, 2018
If an error is encountered during the processing of Immediates, schedule
the remaining queue to finish after all error handling code runs (if the
process is still alive to do so). The new changes make the Immediates
error handling behaviour entirely deterministic and predictable, as the
full queue will be flushed on each Immediates cycle, regardless of
whether an error is encountered or not.

Currently this processing is scheduled for nextTick which can yield
unpredictable results as the nextTick might happen as early as close
callbacks phase or as late as after the next event loop turns Immediates
all fully processed. The latter can result in two full cycles of
Immediates processing during one even loop turn.

The current implementation also doesn't differentiate between Immediates
scheduled for the current queue run or the next one, so Immediates that
were scheduled for the next turn of the event loop, will process
alongside the ones that were scheduled for the current turn.

PR-URL: #17879
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

This does not land cleanly on v9.x, could it be manually backported?

@TimothyGu TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018
apapirovski added a commit to apapirovski/node that referenced this pull request Feb 26, 2018
If an error is encountered during the processing of Immediates, schedule
the remaining queue to finish after all error handling code runs (if the
process is still alive to do so). The new changes make the Immediates
error handling behaviour entirely deterministic and predictable, as the
full queue will be flushed on each Immediates cycle, regardless of
whether an error is encountered or not.

Currently this processing is scheduled for nextTick which can yield
unpredictable results as the nextTick might happen as early as close
callbacks phase or as late as after the next event loop turns Immediates
all fully processed. The latter can result in two full cycles of
Immediates processing during one even loop turn.

The current implementation also doesn't differentiate between Immediates
scheduled for the current queue run or the next one, so Immediates that
were scheduled for the next turn of the event loop, will process
alongside the ones that were scheduled for the current turn.

PR-URL: nodejs#17879
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
If an error is encountered during the processing of Immediates, schedule
the remaining queue to finish after all error handling code runs (if the
process is still alive to do so). The new changes make the Immediates
error handling behaviour entirely deterministic and predictable, as the
full queue will be flushed on each Immediates cycle, regardless of
whether an error is encountered or not.

Currently this processing is scheduled for nextTick which can yield
unpredictable results as the nextTick might happen as early as close
callbacks phase or as late as after the next event loop turns Immediates
all fully processed. The latter can result in two full cycles of
Immediates processing during one even loop turn.

The current implementation also doesn't differentiate between Immediates
scheduled for the current queue run or the next one, so Immediates that
were scheduled for the next turn of the event loop, will process
alongside the ones that were scheduled for the current turn.

Backport-PR-URL: #19006
PR-URL: #17879
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
If an error is encountered during the processing of Immediates, schedule
the remaining queue to finish after all error handling code runs (if the
process is still alive to do so). The new changes make the Immediates
error handling behaviour entirely deterministic and predictable, as the
full queue will be flushed on each Immediates cycle, regardless of
whether an error is encountered or not.

Currently this processing is scheduled for nextTick which can yield
unpredictable results as the nextTick might happen as early as close
callbacks phase or as late as after the next event loop turns Immediates
all fully processed. The latter can result in two full cycles of
Immediates processing during one even loop turn.

The current implementation also doesn't differentiate between Immediates
scheduled for the current queue run or the next one, so Immediates that
were scheduled for the next turn of the event loop, will process
alongside the ones that were scheduled for the current turn.

Backport-PR-URL: #19006
PR-URL: #17879
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
If an error is encountered during the processing of Immediates, schedule
the remaining queue to finish after all error handling code runs (if the
process is still alive to do so). The new changes make the Immediates
error handling behaviour entirely deterministic and predictable, as the
full queue will be flushed on each Immediates cycle, regardless of
whether an error is encountered or not.

Currently this processing is scheduled for nextTick which can yield
unpredictable results as the nextTick might happen as early as close
callbacks phase or as late as after the next event loop turns Immediates
all fully processed. The latter can result in two full cycles of
Immediates processing during one even loop turn.

The current implementation also doesn't differentiate between Immediates
scheduled for the current queue run or the next one, so Immediates that
were scheduled for the next turn of the event loop, will process
alongside the ones that were scheduled for the current turn.

Backport-PR-URL: #19006
PR-URL: #17879
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Feb 27, 2018
@MylesBorins
Copy link
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants