implement queueMicrotask by devsnek · Pull Request #22951 · nodejs/node · GitHub

queueMicrotask is a global from browsers which enables the user code to insert a callback into the microtask queue. We currently do not expose this functionality.

Refs: https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#dom-queuemicrotask

/cc @nodejs/timers @nodejs/open-standards @domenic

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

@devsnek build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/943/pipeline

nit, but lgtm

Code change looks fine. Needs documentation added that preferably includes an explanation of how/why to use this.

Adding a new global is inherently semver-major. Adding this to util first would allow it to go in as semver-minor (which is the same pattern we've used for introducing other bits e.g URL, TextEncoder, TextDecoder, etc)

I'm fine with semver-major.

i'm not sure that it is inherently a semver-major in node; what code would be broken by this?

Adding globals is semver-major as it changes the behavior of non-strict code :/.

Only non-strict code that's omitting var/let/const and using this variable name, though - how likely is that combination?

The policy isn't about likely, it's about possible. A case can be made to the @nodejs/tsc to request a downgrade to semver-minor.

semver-major as this adds a new global

One significant discussion point on this that should be addressed both here and in any accompanying documentation is: Why would someone use this method as opposed to process.nextTick(), what are the similarities, what are the differences, etc. Also, it would be good to get some kind of notion about how this is used in the wild today. Perhaps @domenic could help shed some light on that.

Related intent to ship in Chromium: https://groups.google.com/a/chromium.org/d/msg/blink-dev/LGCEO53Mb6A/9oc4olniBQAJ
Feature dashboard: https://www.chromestatus.com/feature/5111086432911360

This is not documented in MDN yet, the only existing extensive documentation seem to be the spec itself (which is not normative i.e. no pressure to follow this spec).

I'm general you should always use queueMicrotask. only use nextTick if you for some weird reason need to jump ahead of the microtask queue. I believe @bmeck at some point gave the example of throwing an error in the next tick but before microtasks run.

What browsers support this? A quick check indicates that none of Chrome, Safari, and Firefox supports this yet, at least not as a global. Nor can I find it mentioned on a MDN page. Whereas TC39 has a well defined process with stages of proposals, the WhatWG has no such process.

If this is not widely supported by browsers, I would suggest that the feature be marked as experimental as the spec may change as feedback from browsers is received.

If this is not widely available in browsers, could you attach it to process instead? Also, flagging it as experimental would be better.

Adding globals is semver-major as it changes the behavior of non-strict code :/.

Can you add a test that shows the relationship with process.nextTick and setImmediate?

setImmediate and nextTick are tested separately, and there is a PR open to change the semantics of how microtasks and timers interleave so I think it would be best to leave that alone from this side.

I think it should be tested. It's fairly important that we make it explicit so we verify this all the time.

Can you link the PR that changes the semantics of how microtasks and timers interleave?

#22842

Part of this PR's intentions is interoperability... I'm not going to land it on util or process or whatever.

As far as I can tell, the reason this isn't in any browsers yet is because it is fairly new. It only passed TAG review like 20 days ago. @joyeecheung posted some links above which are fairly helpful in this regard.

If we choose the experimental route, I would prefer a warning. Would anyone want a flag instead?

If we choose the experimental route, I would prefer a warning. Would anyone want a flag instead?

A warning should be the way to go.

+1 for experimental w/warning and +1 for requiring a flag while this is still not shipped in any browsers.

I'm general you should always use queueMicrotask. only use nextTick if you for some weird reason need to jump ahead of the microtask queue.

Well, until this is proven out with solid implementation experience over time, I'm not sure that's the advice we should be giving generally, but I definitely get the idea. This should definitely be added to globals.md with a clear explanation of the motivation behind it.

Needs to go before ## require() ABC-wise.

`queueMicrotask` -> `queueMicrotask()` here and below.

`error` event -> `'error'` event

`process.nextTick` -> `process.nextTick()` here and in mentions below.

This is going to need more of an explanation as to why it is the "idiomatic choice". process.nextTick() has been around a very very long time. Simply introducing this is not going to get users to move over to queueMicrotask() unless the differences between the two are more spelled out. Given that users of process.nextTick() are generally very familiar with it's timing, I'm not sure that "unexpected order" is correct.

The example is good, but including a version of the example and including a comparison of the output to help illustrate the point is better.

load event -> 'load' event

Maybe it is worth to replace d with a more meaningful name?

nit: queueMicrotask() is experimental

@jasnell @mcollina is the async_hook integration looking correct?

is the async_hook integration looking correct?

@AndreasMadsen and @trevnorris are the best to answer that question :-)

I'm general you should always use queueMicrotask. only use nextTick if you for some weird reason need to jump ahead of the microtask queue.

I feel that we need to be careful about the cognitive cost that we add to our API in exchange to an utility like this. How many of our users have a clear idea about what the micro task queue is in the first place? Isn't this supposed to be an implementation detail that most users should not care about? (we are already paying the cost with the name process.nextTick()) For a beginner to Node.js, or to JavaScript in general, how are we supposed to explain to them what a micro task queue is and why this API is a preferable way to make a task async? Do we redirect them to...the ECMAScript specification? The HTML spec? That would make the Node.js API even more intimidating than it already is.

Also worth reiterating: process.nextTick() will continue to exist long after this is added... meaning that as far as a 'cognitive cost' perspective is concerned, this just becomes yet another thing people need to learn and consider when comparing nextTick(), setImmediate(), and Promise timing.

@joyeecheung @jasnell good points. i don't really have answers to how developer education should happen.

The main point here is that when you want to do something in the next tick it should happen the same way everything else does. People who care about this timing generally use Promise.resolve().then(f) or MutationObserver which I hope we can agree is more of a cognitive overhead than queueMicrotask.

If the name itself is a problem, maybe we could talk to WHATWG about changing it? nothing else is shipping this right now so it would be fairly easy. I think the API itself stands by its own merit, regardless of the name it goes by.

Also worth reiterating: process.nextTick() will continue to exist long after this is added... meaning that as far as a 'cognitive cost' perspective is concerned, this just becomes yet another thing people need to learn

I know that this is going to sound like a strange question, but here goes: Why?

At the moment, Node is steadily acquiring cruft with no discernible plan to get to a cleaner place over time. I do believe that there is a viable alternative, and I've seen it operate in practice with a popular framework and a language that has very similar characteristics to JavaScript. I sketch out the approach in The menu is omakase.

I don't believe that any one change justifies the approach I've outlined. It only becomes viable if you look at the cumulative effect of both the real an cognitive costs of maintaining multiple similar APIs and/or the cost of making it more difficult or less attractive for Node to implement standards.

The TL;DR version of this for the queueMicrotask() API is that an alternative to implementing it as an experimental feature for Node 10 or 11, consider having it start life as an nom module that can be installed today on Node 8 or higher. It can then be pulled into core when ready. And most interesting, we can discuss extracting process.nextTick() into a module in, say, the node 14 timeframe.

@rubys like i said above, part of the reason for choosing this specific api is interoperability with what the rest of the ecosystem has planned. I'm sure people will have polyfills anyway for older browsers and older node, but i think node core should ship this feature.

This looks like it landed without a green CI.

After-the-fact, but here we go anyway...

Rebuild arm fanned: https://ci.nodejs.org/job/node-test-commit-arm-fanned/3719/

Rebuild custom worker should eventually show up at: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/1096/

@Trott would this be a bug? neither the github ui or git-node caught these failures.

Always best to directly check the ci page itself. Don't rely on the reporting to GitHub. It can be quite unreliable at times

@devsnek git-node does not actually check the CI status, it only checks whether the last CI is older than the last commit in the PR, given how unreliable the CI is.

ncu-ci does help analyzing the CI status, though (by guessing the link to the last CI in a thread and then using the Jenkins API to check all the builds), but it's still not able to understand the manual rebuilds. Maybe we could incorporate that into git-node...

Arm rebuild is green. Custom/worker rebuild is red but I'm not sure why, at least not after a quick glance. Anyone know? https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/1096/

Arm rebuild is green. Custom/worker rebuild is red but I'm not sure why, at least not after a quick glance. Anyone know? https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/1096/

not ok 154 parallel/test-bootstrap-modules
  ---
  duration_ms: 0.335
  severity: fail
  exitcode: 1
  stack: |-
    
    events.js:167
          throw er; // Unhandled 'error' event
          ^
    AssertionError [ERR_ASSERTION]: Internal Binding contextify,Internal Binding worker,NativeModule events,Internal Binding trace_events,NativeModule internal/safe_globals,NativeModule internal/async_hooks,NativeModule internal/errors,Internal Binding uv,Binding buffer,Internal Binding async_wrap,Binding config,Binding icu,NativeModule util,NativeModule internal/validators,NativeModule internal/encoding,NativeModule internal/util,Binding constants,Internal Binding util,NativeModule internal/util/types,Internal Binding types,NativeModule buffer,NativeModule internal/buffer,NativeModule internal/process/per_thread,NativeModule internal/process/worker_thread_only,NativeModule internal/process/stdio,NativeModule internal/worker,NativeModule assert,NativeModule internal/assert,NativeModule fs,NativeModule path,NativeModule internal/constants,Binding fs,NativeModule internal/fs/utils,NativeModule internal/url,NativeModule internal/querystring,Internal Binding url,NativeModule stream,NativeModule internal/streams/pipeline,NativeModule internal/streams/end-of-stream,NativeModule internal/streams/legacy,NativeModule _stream_readable,NativeModule internal/streams/buffer_list,NativeModule internal/streams/destroy,NativeModule internal/streams/state,NativeModule _stream_writable,NativeModule _stream_duplex,NativeModule _stream_transform,NativeModule _stream_passthrough,Internal Binding messaging,Internal Binding symbols,NativeModule internal/error-serdes,NativeModule v8,Internal Binding serdes,Internal Binding v8,NativeModule url,NativeModule internal/process/warning,NativeModule internal/process/next_tick,NativeModule internal/process/promises,NativeModule internal/fixed_queue,Internal Binding performance,NativeModule internal/inspector_async_hook,Binding inspector,Internal Binding options,NativeModule timers,Internal Binding timers,NativeModule internal/linkedlist,NativeModule internal/priority_queue,NativeModule internal/timers,NativeModule internal/modules/cjs/loader,NativeModule vm,NativeModule internal/modules/cjs/helpers,NativeModule console,NativeModule internal/queue_microtask,NativeModule async_hooks,NativeModule internal/domexception,NativeModule worker_threads,NativeModule module
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/parallel/test-bootstrap-modules.js:14:1)
        at Module._compile (internal/modules/cjs/loader.js:703:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:714:10)
        at Module.load (internal/modules/cjs/loader.js:613:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:552:12)
        at Function.Module._load (internal/modules/cjs/loader.js:544:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:756:12)
        at MessagePort.port.on (internal/worker.js:447:27)
        at MessagePort.emit (events.js:182:13)
        at MessagePort.onmessage (internal/worker.js:67:8)
    Emitted 'error' event at:
        at Worker.[kOnErrorMessage] (internal/worker.js:319:10)
        at Worker.[kOnMessage] (internal/worker.js:329:37)
        at MessagePort.Worker.(anonymous function).on (internal/worker.js:266:57)
        at MessagePort.emit (events.js:182:13)
        at MessagePort.onmessage (internal/worker.js:67:8)
  ...

So that failure in workers is related to this change.

This needs to be reverted or fixed.

Specifically:

Compile 59a8324 and tools/test.py --worker test/parallal/test-bootstrap-modules.js passes.

Next commit is the one landed here, de0441f. Compile there and run the same command and it fails.

It's because that commit adds a new module loaded at node.js start up, which can vary from platform to platform and on how node.js is started. It's not a great test and is very brittle

Fix from @devsnek at #23046

Notable changes: * Build * FreeBSD 10 is no longer supported. [#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * `crypto` * PEM-level encryption is now supported. [#23151](#23151) * An API for key pair generation has been added. [#22660](#22660) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * `http2` * An event will be emitted when a `PING` frame is received. [#23009](#23009) * Support for the `ORIGIN` frame has been added. [#22956](#22956) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Promises * A new `multipleResolves` event will be emitted when a Promise is resolved (or rejected) more than once. [#22218](#22218) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[nodejs#22617](nodejs#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [nodejs#21316](nodejs#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [nodejs#21649](nodejs#21649) * `console.time()` will no longer reset a timer if it already exists. [nodejs#20442](nodejs#20442) * Dependencies * V8 has been updated to 7.0. [nodejs#22754](nodejs#22754) * `fs` * The `fs.read()` method now requires a callback. [nodejs#22146](nodejs#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[nodejs#20735](nodejs#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [nodejs#20270](nodejs#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [nodejs#22951](nodejs#22951) * Internal * Windows performance-counter support has been removed. [nodejs#22485](nodejs#22485) * The `--expose-http2` command-line option has been removed. [nodejs#20887](nodejs#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [nodejs#20002](nodejs#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [nodejs#22281](nodejs#22281) * `util.inspect()` output size is limited to 128 MB by default. [nodejs#22756](nodejs#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [nodejs#21914](nodejs#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](nodejs/node#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](nodejs/node#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](nodejs/node#20442) * Dependencies * V8 has been updated to 7.0. [#22754](nodejs/node#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](nodejs/node#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](nodejs/node#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](nodejs/node#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](nodejs/node#22951) * Internal * Windows performance-counter support has been removed. [#22485](nodejs/node#22485) * The `--expose-http2` command-line option has been removed. [#20887](nodejs/node#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](nodejs/node#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](nodejs/node#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](nodejs/node#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](nodejs/node#21914)

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. tools Issues and PRs related to the tools directory. labels Sep 19, 2018

devsnek added lib / src Issues and PRs related to general changes in the lib or src directory. and removed tools Issues and PRs related to the tools directory. labels Sep 19, 2018

bmeck approved these changes Sep 19, 2018

View reviewed changes

Comment thread test/parallel/test-queue-microtask.js Outdated

jasnell requested changes Sep 19, 2018

View reviewed changes

jasnell reviewed Sep 19, 2018

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 19, 2018

mcollina reviewed Sep 19, 2018

View reviewed changes

devsnek force-pushed the feature/queue-microtask branch from 21ae3a5 to d1d53d9 Compare September 19, 2018 19:44

vsemozhetbyt reviewed Sep 19, 2018

View reviewed changes

jasnell reviewed Sep 19, 2018

View reviewed changes

Comment thread test/parallel/test-queue-microtask.js Outdated

jasnell reviewed Sep 19, 2018

View reviewed changes

targos removed the dont-land-on-v10.x label Sep 23, 2018

joyeecheung mentioned this pull request Sep 25, 2018

CI failures: 20180925 nodejs/reliability#17

Closed

jasnell mentioned this pull request Oct 2, 2018

11.0.0 proposal #23230

Closed

4 tasks

benjamingr mentioned this pull request Oct 16, 2018

add support for queueMicrotask sinonjs/fake-timers#217

Merged

codebytere added the dont-land-on-v10.x label Nov 29, 2018

codebytere mentioned this pull request Nov 29, 2018

test: fix flaky VM timeout test on Raspberry Pi #24238

Closed

3 tasks

Ziphwy mentioned this pull request Nov 11, 2020

NodeJS 系列之事件循环 Ziphwy/Blog#10

Open

qufei1993 mentioned this pull request Oct 20, 2021

JavaScript 异步编程指南 - 聊聊 Node.js 中的事件循环 qufei1993/blog#21

Open

implement queueMicrotask by devsnek · Pull Request #22951 · nodejs/node · GitHub

Uh oh!

Uh oh!

implement queueMicrotask - #22951

implement queueMicrotask#22951devsnek merged 0 commit intonodejs:masternodejs/node:masterfrom devsnek:feature/queue-microtaskdevsnek/node:feature/queue-microtaskCopy head branch name to clipboard

Conversation

devsnek commented Sep 19, 2018

Checklist

Uh oh!

nodejs-github-bot commented Sep 19, 2018

Uh oh!

bmeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Sep 19, 2018

Uh oh!

jasnell commented Sep 19, 2018

Uh oh!

joyeecheung commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devsnek commented Sep 19, 2018

Uh oh!

rubys commented Sep 19, 2018

Uh oh!

mcollina commented Sep 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devsnek commented Sep 19, 2018

Uh oh!

mcollina commented Sep 19, 2018

Uh oh!

jasnell commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

implement queueMicrotask#22951
devsnek merged 0 commit into
nodejs:masterfrom
devsnek:feature/queue-microtask

joyeecheung commented Sep 19, 2018 •
edited
Loading

jasnell commented Sep 19, 2018 •
edited
Loading

joyeecheung commented Sep 19, 2018 •
edited
Loading

joyeecheung commented Sep 24, 2018 •
edited
Loading