lib: move queueMicrotask to stable #25594

devsnek · 2019-01-20T16:29:00Z

IIRC the critera for stable was better support from other platforms, which has since been fulfilled by safari and chrome.

Closes #25592

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2019-01-20T16:29:02Z

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

mcollina · 2019-01-20T16:56:19Z

Is this compatible with the other implementation? If so, have we got tests in wpt?

devsnek · 2019-01-20T17:04:10Z

Looks like this exists: https://github.com/web-platform-tests/wpt/tree/master/html/webappapis/microtask-queuing

@joyeecheung how should i go about integrating the tests here?

joyeecheung · 2019-01-21T11:01:03Z

@devsnek The only test in the WPT that can be run in Node without modification is https://github.com/web-platform-tests/wpt/blob/master/html/webappapis/microtask-queuing/queue-microtask.any.js (the other two either needs document.createElement or window.addEventListener)so I would not mind if you just port it by hand, but you can also follow the instructions in https://github.com/nodejs/node/tree/master/test/wpt#how-to-add-tests-for-a-new-module (though I am not sure if the current tooling work on tests under html/webappapis as they do not support non-top-level directories yet, I could work on that though)

mcollina

LGTM

Trott · 2019-01-21T14:36:38Z

Is test/known_issues/test-vm-timeout-escape-queuemicrotask.js a queuemicrotask bug, a vm timeout bug, or a V8 bug? I'm guessing that it's not a queuemicrotask bug, and therefore not worth holding this up over, but leaving this comment in case I'm wrong.

addaleax · 2019-01-21T14:39:02Z

@Trott It’s a vm “bug” that requires currently not existing V8 APIs to be fixed. Tbh, I don’t think it will ever be worth the effort to properly fix it and we should just leave it alone, and Workers might fill the gap well enough.

In any case, yes, it’s definitely not a queueMicrotask bug.

sam-github · 2019-01-21T17:00:39Z

I'm not sure if this is the right place, but I would like to suggest that the documentation be enhanced to describe why Node.js has this API, and/or under what conditions we think it should be used.

https://nodejs.org/api/globals.html#globals_queuemicrotask_callback is ATM a purely factual description of what it does, but given the difficulty in knowing when to use setImmediate() vs nextTick(), adding a third option seems to just add to the confusion.

I'm personally confused, because while the docs don't say anything on expected usage, there is some example code, that is virtually identical in purpose to the examples in process.nextTick, but with the additional mention of something about interaction with promises. The example just makes me wonder if all usages of nextTick somehow don't interact with promises, or perhaps the example is not specific enough.

See https://nodejs.org/api/process.html#process_process_nexttick_callback_args to compare the examples, and description.

For me, this kind of vagueness is fine for experimental APIs, but once they have had some use, it should be clarified.

Its possible the docs should be something like:

This API allows access to the same callback queue used by promises. Most Node.js code should be using nextTick, but implementors of promise utility libraries may need to schedule callbacks with precise ordering relationships with respect to promises. It may also be useful for running non-Node.js specific javascript code, since the API also exists in browsers (ref to browser docs....).

If the above guess at the purpose is somewhat correct, the example should not be so similar to the nextTick example, but instead be a Promise.waitAll() implementation, or something of the like.

devsnek · 2019-01-21T17:39:28Z

It originally said something along the lines of "This API schedules a callback interleaved in the microtask queue, interleaved with promsies. Most Node.js code should use this API for scheduling. If you want to guarantee your callback runs before promises, use process.nextTick" but iirc @bnoordhuis removed it?

sam-github · 2019-01-21T17:47:35Z

Most Node.js code should use this API for scheduling.

If this is true, then the process.nextTick docs need to be modified to reflect that. As it is, we seem to have two different APIs that "should" be used for the same purpose, quite confusing.

mcollina · 2019-01-21T17:59:55Z

Most Node.js code should use this API for scheduling.

I disagree. Using this API triggers a costly JS <-> C++ transition, while nextTick happens only in JS-land. I expect a significant performance different between the two. IMHO nextTick should be the preferred way, wit queueMicrotask being used for compat reason.

devsnek · 2019-01-21T18:05:07Z

@mcollina if performance is a concern we can definitely work on that, but nextTick jumps out of line and the zalgo of that isn't usually needed.

sam-github · 2019-01-21T18:15:55Z

@devsnek There are two lines. It would be equally true to say promises jump out of line. Since the nextTick line is used extensively inside node.js (not to mention the wider set of node.js user code), encouraging people to use a different queue seems more likely to create problems than to create consistency. Note for those not familiar: zalgo.

The core of the problem is that promises created a new queue, and now there are two. I assume having the utask queue be integrated with the nexttick queue wasn't technically possible? I had the impression that v8 went through some effort to give embedders control over utask scheduling.

And how does setImmediate fit into this? Didn't it come from the browser? Did the promises queue unleash zalgo in pure browser javascript, or did browsers retarget setImmediate at the utask queue?

jasnell · 2019-01-21T18:18:55Z

I'm -1 on any language that suggests that this be favored over nextTick for a number of reasons that have already been discussed here. We should say only what the differences between them are without placing any relative value or preference.

joyeecheung · 2019-01-21T18:56:25Z

I opened #25616 to run the WPT in core - to my surprise no additional modification to the current tooling is necessary to add this into core

sam-github · 2019-01-21T20:04:32Z

I'm a little uncomfortable with not specifying a preference, I think our API users deserve to be informed not only about how the APIs work, but how we expect them to be used. But I can see it is contentious.

Perhaps we can at least acknowlege the tradeoffs between nextTick and and queueMicrotask, either in the API docs here, or we could link to the guide, and enhance it with information about the new queue.
https://nodejs.org/en/docs/guides/event-loop-timers-and-nexttick/

And despite the title of that guide, I notice it isn't linked to from the nextTick docs. I'll add it.

lpinca · 2019-02-09T17:05:41Z

lib/internal/bootstrap/node.js

@@ -761,30 +761,12 @@ function setupGlobalEncoding() {
 }

 function setupQueueMicrotask() {
+  const { queueMicrotask } =
+    NativeModule.require('internal/queue_microtask');
  Object.defineProperty(global, 'queueMicrotask', {


We can do

global.queueMicrotask = queueMicrotask;

now no?

we can, but it's best to be explicit.

We do it for setTimeout, setImmediate, etc. I'm fine with it as is though.

Making it global means semver-major.

it's already a global

Ah! Right lol... Had forgotten that :)

I personally prefer

global.queueMicrotask = queueMicrotask;

It is significantly shorter and the implications should be clear.

BridgeAR · 2019-03-05T23:59:40Z

lib/internal/bootstrap/node.js

@@ -761,30 +761,12 @@ function setupGlobalEncoding() {
 }

 function setupQueueMicrotask() {
+  const { queueMicrotask } =
+    NativeModule.require('internal/queue_microtask');
  Object.defineProperty(global, 'queueMicrotask', {


I personally prefer

global.queueMicrotask = queueMicrotask;

It is significantly shorter and the implications should be clear.

BridgeAR · 2019-03-06T00:01:03Z

@sam-github I agree that the docs could and probably should be improved further but to me it seems that can be done in a separate PR as this should mainly just move the queueMicrtask to stable without changing the documentation.

@devsnek this needs a rebase.

sam-github · 2019-03-06T00:10:42Z

I think its odd to move something from experimental when we don't have consensus on what purpose it has, but its stable in the sense of "not going to change", so sure, +0.

devsnek · 2019-03-06T04:59:36Z

the two points were parity with browsers and providing an api that lets people enter the proper async queue.

i feel like this api should be explained as "if you don't know why you need nextTick, use queueMicrotask."

"why do i need nextTick" is probably legacy reasons or performance (we can get rid of the perf issue entirely by using %EnqueueMicrotask instead of calling c++)

sam-github · 2019-03-06T17:35:14Z

Unfortunately, there is also a sizable "if you don't know why you need queueMicrotask, use nextTick" camp. And also a ... use setImmediate and ... use setTimeout(0) camp (though that last camp is hopefully being abandoned by its last inhabitants).

devsnek · 2019-03-06T18:28:46Z

there is also a sizable "if you don't know why you need queueMicrotask, use nextTick" camp

no one has yet given a reason why new code should use nextTick over queueMicrotask besides performance. I understand there are legacy timing reasons to use nextTick, but that seems to fall squarely in the category of "I know why I need nextTick"

some reasons to use queueMicrotask:

the async primitive of js is a promise, so it makes sense to engage with that queue, not platform specific ones.
supported in multiple platforms

BridgeAR · 2019-03-06T19:51:56Z

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

PR-URL: nodejs#26520 Fixes: nodejs#26528 Refs: nodejs#25594 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #26520 Fixes: #26528 Refs: #25594 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

mcollina · 2019-03-14T18:08:57Z

This breaks readable-stream test suite. Please don't include it in a release as it would need to be addressed or reverted.

devsnek · 2019-03-14T18:29:46Z

@mcollina can you provide some more info on that?

mcollina · 2019-03-14T18:42:10Z

The tests of readable-stream all comes out red. It might be something in the harness.

targos · 2019-03-14T18:43:49Z

What I saw in citgm is that the presence of the new global makes the tests throw an error

devsnek · 2019-03-14T19:08:06Z

is there some common test framework or something that is failing? if something needs to be updated we should make sure it is updated. I added queueMicrotask to the globals lib a while ago (sindresorhus/globals#142)

targos · 2019-03-14T20:15:13Z

It's from here: https://github.com/nodejs/readable-stream/blob/5b90ed2c1141fc41cb1f612eb88846473369298d/test/common/index.js#L315-L321

devsnek · 2019-03-14T20:30:59Z

seems like it just needs to be modified to collect globals at startup instead of using a known list cc @mcollina

joyeecheung · 2019-03-15T00:00:36Z

That file looks like a port of our test/common/index.js, maybe it just needs an update?

mcollina · 2019-03-15T07:19:02Z

Yes. However the switch of the enumerable flag makes this change semver-major in nature as it’s going to broke some module CI systems. I think it would be safer if this is backported to 11 with the enumerable flag disabled.

devsnek added the experimental Issues and PRs related to experimental features. label Jan 20, 2019

devsnek requested a review from a team January 20, 2019 16:29

devsnek force-pushed the queuemicrotask-stable branch 2 times, most recently from 49f907d to 156cd59 Compare January 20, 2019 16:54

mcollina approved these changes Jan 21, 2019

View reviewed changes

jasnell approved these changes Jan 21, 2019

View reviewed changes

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 27, 2019

lpinca reviewed Feb 9, 2019

View reviewed changes

BridgeAR approved these changes Mar 5, 2019

View reviewed changes

joyeecheung mentioned this pull request Mar 8, 2019

test: bump test-bootstrap-modules.js limit #26520

Merged

3 tasks

BridgeAR added dont-land-on-v6.x labels Mar 14, 2019

lib: move queueMicrotask to stable #25594

lib: move queueMicrotask to stable #25594

Conversation

devsnek commented Jan 20, 2019

Checklist

nodejs-github-bot commented Jan 20, 2019

mcollina commented Jan 20, 2019

devsnek commented Jan 20, 2019

joyeecheung commented Jan 21, 2019 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

Trott commented Jan 21, 2019

addaleax commented Jan 21, 2019

sam-github commented Jan 21, 2019

devsnek commented Jan 21, 2019

sam-github commented Jan 21, 2019

mcollina commented Jan 21, 2019

devsnek commented Jan 21, 2019

sam-github commented Jan 21, 2019

jasnell commented Jan 21, 2019

joyeecheung commented Jan 21, 2019

sam-github commented Jan 21, 2019

lpinca Feb 9, 2019 • edited Loading

Choose a reason for hiding this comment

devsnek Feb 9, 2019

Choose a reason for hiding this comment

lpinca Feb 9, 2019

Choose a reason for hiding this comment

jasnell Feb 9, 2019

Choose a reason for hiding this comment

devsnek Feb 9, 2019

Choose a reason for hiding this comment

jasnell Feb 9, 2019

Choose a reason for hiding this comment

BridgeAR Mar 5, 2019

Choose a reason for hiding this comment

BridgeAR Mar 5, 2019

Choose a reason for hiding this comment

BridgeAR commented Mar 6, 2019

sam-github commented Mar 6, 2019

devsnek commented Mar 6, 2019

sam-github commented Mar 6, 2019

devsnek commented Mar 6, 2019 • edited Loading

BridgeAR commented Mar 6, 2019

mcollina commented Mar 14, 2019

devsnek commented Mar 14, 2019

mcollina commented Mar 14, 2019

targos commented Mar 14, 2019

devsnek commented Mar 14, 2019

targos commented Mar 14, 2019

devsnek commented Mar 14, 2019

joyeecheung commented Mar 15, 2019 • edited Loading

mcollina commented Mar 15, 2019

joyeecheung commented Jan 21, 2019 •

edited

Loading

lpinca Feb 9, 2019 •

edited

Loading

devsnek commented Mar 6, 2019 •

edited

Loading

joyeecheung commented Mar 15, 2019 •

edited

Loading