AsyncWrap #9
Comments
|
From prior discussions the direction we were heading with this was:
/cc'ing @ofrobots as well, as I've pinged him to take a look at the proposed approach. I'm labeling this as "blocks unflagging" for now, but will change it if folks object. |
|
👍 @chrisdickinson this sounds reasonable. |
Is this to say that making AsyncWrap public API is blocked because we don't have the API available to make it work correctly with the MicrotaskQueue? |
|
@trevnorris That was my understanding, yes — since a public AsyncWrap API user wouldn't be able to rely on the ability to back-associate asynchronous events in the presence of native Promise use in their extended dependency chain without modifications to the microtask queue — |
|
@chrisdickinson I'd never planned on holding off making AsyncWrap public on points we can't control. With the work you and others have done to fix this there may be a solution in the near to mid term, but unlike timers and nextTick (which are also not currently supported but on me to implement) there isn't a directly supported API I can use. This being said, it's not like AsyncWrap would go public tomorrow, or even next month, but I have a feeling that there will always be something in the pipe that won't work properly with AsyncWrap. (e.g. if ever those strange Promise based streams in muddy black magic come to node) |
|
@trevnorris Understandable. Would it be fair to say that ideally we'd have a resolution for MicrotaskQueue control before exposing AsyncWrap? (Notably: having a solution for the microtask queue would solve the problem of WHATWG streams as well, since those are based on top of Promises.) |
|
Just as an FYI, I've a PR open against AndreasMadsen's "async-hook" wrapper for a Promise patch (AndreasMadsen/async-hook#2). It's obviously incredibly lacking, but was how I attempted to tackle it with what's available. Very cool to see the microtask work happening. |
|
@chrisdickinson custom microtask queue wouldn't be enough if we want to capture context on |
That would be ideal, and would also address the core API gap of queue'ing native callbacks in a nextTick way (i.e. Aside on this note, how would AsyncWrap work with the MicrotaskQueue executor from the native side? I believe some enhancements will need to be placed in AsyncWrap, and am will to work that out once this moves forward. |
|
@vkurchatkin At this point I'm not sure we must do that. d = domain.create()
d.run(() => {
return fs.readFilePromise()
}).then(data => {
// runs in d, readFilePromise's executor was run in d, queued `readFile` in d, microtask
// enqueued by callback of `readFile`
return fs.existsPromise() // executor + callback are bound to d, resolution runs in d
.then(() => {
// still bound to d, since the microtask is queued inside of `existsPromise`
})
}).then(otherData => {
// runs in d, since resolution of dependent promise of `existsPromise` is
// run in d and enqueues microtask there
})
// escapes:
const p = d.run(() => {
return fs.readFilePromise()
})
setTimeout(() => {
p.then(() => {
// not run in d; microtask enqueued by "p.then"
})
}, 500)Only handlers added after the resolution of a Node core-returned Promise would pick up the domain of their enclosing scope. While this is probably going to be a bit difficult to document, I believe it should be workable behavior. |
|
/cc @nodejs/tracing as I completely missed this myself. |
|
@trevnorris, Can I get some feedback on this https://bugs.chromium.org/p/v8/issues/detail?id=4643#c12? Thanks! |
|
@gsathya Sorry for the delay. While hiding from the internet and focusing on the implementation realized my previous ask was incorrect, and what's actually needed is simpler. Hopefully you agree. |
|
Hi @trevnorris, commenting here as to not clutter the V8 issue. I believe the way you're describing the integration of Promises differs from at least I'd like to see (justification follows). Maybe it's a limitation with how microtasks are scheduled, but figured I should at least say something for there to be any chance of change. Per your example: setTimeout(function timeout01() {
const p = new Promise(function promise01(res) {
setTimeout(function timeout02() {
res('foo');
});
});
p.then(function then01(val) {
return 'bar';
});
p.then(function then02(val) {
return 'baz';
});
});From here we could say any resolved promise is a handle, which runs for each PromiseReactionJob. And thus if we scheduled a reaction later, we would still find ourselves with that resolved promise as the parent. setTimeout(function timeout03() {
p.then(function then03(val) {
return 'quux';
});
}, 3600 * 1000);The problem I find with this is when it comes to things such as request tracking in an APM / async context ala CLS. If you ever share a Promise between two requests, you jump contexts. Here's a simple example (where I make assumptions about a working version of nodejs/node#5573 which I haven't gotten back to - sorry): const http = require('http');
let promiseToCachedThing = null;
function getThingAndCacheItAsPromise() {
if (promiseToCachedThing === null) {
promiseToCachedThing = new Promise(function promise01(resolvePromise01) {
setTimeout(function timeout01() {
resolvePromise01(Buffer.from('howdy!', 'utf8'));
});
});
}
return promiseToCachedThing;
}
http.createServer(function requestHandler(req, res) {
getThingAndCacheItAsPromise()
.then(function returnCachedThing(thing) {
res.statusCode = 200;
res.end(thing);
}):
}).listen(0);In this case we've completely lost our context of request02. If we go back to our original example and change the timing to be like this instead: Then our multi-request example ends up looking like so: I hope that makes sense. Do you have thoughts on that? Do you think it would be a possibility? Thanks |
|
@omsmith Thanks for the feedback. While revising my model of how Promises should work with async hooks I decided to model it on how callbacks, and events, are currently evaluated. While doing so I realized that it may need to be better communicated that Let's take a quick look at node's API as an reference: let server = null;
setTimeout(function timer01() {
server = net.createServer().listen(8080);
}, 10);
setTimeout(function timer02() {
server.on('connection', c => {
print('connection made');
});
}, 100);Both Thus far the API has evaluated the |
|
I would definitely expect the parent to be server in that example. I like exactly what you said: Thus far the API has evaluated the parentId as the handle responsible for the new class' construction. I suppose what I'm asking is for a PromiseReaction to also be a handle which is created when |
|
@omsmith Thanks. The first time I spec'd how Promises should interact with async_hooks it was very similar to what you described. Though as I was finishing up implementing the embedder API I realized there was an issue with maintaining an asynchronous stack trace. Example code: 'use strict';
const crypto = require('crypto');
let p = null;
setTimeout(function sT01() {
p = new Promise(function executor(res) {
crypto.randomBytes(1024, function bytes(data) {
res(data);
});
});
}, Math.random() * 60 * 60 * 1000); // max of 1 min
setInterval(function sI02() {
p.then(function thenFn(val) {
console.log(val.toString('hex'));
});
clearInterval(this);
}, 10 * 1000); // check every 10 secondsMake the assumption that we're recording the stack trace every time Can see that there's no record of the call to The rational here is that Though I label this event emitter behavior an implementation detail. The following is not pretty, but does demonstrate a simple event emitter that behaves in a similar way to a 'use strict';
function MyEmitter() {
// Use Map so the event can be anything.
this.listeners = new Map();
this.events = new Map();
}
MyEmitter.prototype.on = function on(event, listener) {
if (!this.listeners.has(event))
this.listeners.set(event, [listener]);
else
this.listeners.get(event).push(listener);
// Now check if events already exist for this listener
if (this.events.has(event)) {
for (let value of this.events.get(event))
listener.call(this, value);
}
};
MyEmitter.prototype.emit = function emit(event, value) {
// Call previous listeners if they exist for the event.
if (this.listeners.has(event)) {
const listeners = this.listeners.get(event);
for (let listener of listeners)
listener.call(this, value);
}
// Then add the event to the event map.
if (this.events.has(event)) this.events.get(event).push(value);
else this.events.set(event, [value]);
};
const me = new MyEmitter();
me.on('foo', function onFoo(value) { console.log('onFoo', value) });
me.emit('bar', 'emit bar');
me.on('bar', function onBar(value) { console.log('onBar', value) });
me.emit('foo', 'emit foo');
setTimeout(() => {
me.on('foo', val => console.log('foo timeout', val));
me.on('bar', val => console.log('bar timeout', val));
}, 1000);Of course another difference is that a Promise executor is only ever resolved once. So it's impossible that two Anyway, to recap. The short of it is that I view Thoughts? |
|
Another way to put it is that the linkage async_wrap is intended to provide is the edges of the synchronous JS execution barriers. The By pushing the event trigger timing as close as possible to the async edges, it ensures the most complete encapsulation of the JS execution within a given context. |
|
If you have 10 steps of process for every purchase request in a company, who's responsible for the work that's created from that - the one submitting the purchase request, or the one who made the step of process? :) I could see I appreciate the insightful and very respectful discussion @trevnorris and @Qard. It's especially nice to know you at least had the same train of thought at some point! |
|
Transaction tracing is definitely and important purpose for |
|
|
|
It looks like something changed between when this issue was last discussed here, and what actually ended up in 8.0.0. Here is an example: const async_hooks = require('async_hooks');
const fs = require('fs')
let indent = 0;
async_hooks.createHook({
init(asyncId, type, triggerId, obj) {
const cId = async_hooks.currentId();
fs.writeSync(1, ' '.repeat(indent) +
`${type}(${asyncId}): trigger: ${triggerId} scope: ${cId}\n`);
},
before(asyncId) {
fs.writeSync(1, ' '.repeat(indent) + `before: ${asyncId}\n`);
indent += 2;
},
after(asyncId) {
indent -= 2;
fs.writeSync(1, ' '.repeat(indent) + `after: ${asyncId}\n`);
},
destroy(asyncId) {
fs.writeSync(1, ' '.repeat(indent) + `destroy: ${asyncId}\n`);
},
}).enable();
new Promise(function (resolve) {
setTimeout(function () {
fs.writeSync(1, ' '.repeat(indent) + `current id at resolve: ${async_hooks.currentId()}\n`);
fs.writeSync(1, ' '.repeat(indent) + `trigger id at resolve: ${async_hooks.triggerId()}\n`);
resolve();
});
}).then(() => {
async_hooks.triggerId()
fs.writeSync(1, ' '.repeat(indent) + `current id in then: ${async_hooks.currentId()}\n`);
fs.writeSync(1, ' '.repeat(indent) + `trigger id in then: ${async_hooks.triggerId()}\n`);
});which prints: Given this, I don't see any way to link the context at the time of resolve (3) to the execution of the then callback (5). I would have expected trigger id during the callback passed to then (5) to be 3 |
|
Isn't this what nodejs/node#13367 is about? |
|
Yes, sorry. I missed it initially, because I think the ticket/PR was initially about something different, but looking through it, it looks like the above use cases gets touched on further down in the comments. |
Native promises don't have hooks in V8 so interop with AsyncWrap is non-trivial.
@trevnorris
The text was updated successfully, but these errors were encountered: