async_hooks: support promise resolve hook #15296

addaleax · 2017-09-09T16:04:33Z

Add a promiseResolve() hook.

/cc @nodejs/diagnostics @nodejs/async_hooks

Checklist

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

Affected core subsystem(s)

async_hooks

vsemozhetbyt · 2017-09-09T16:08:44Z

doc/api/async_hooks.md

+
+// promiseResolve is called only for promise resources, when the
+// `resolve` function passed to the `Promise` constructor is invoked
+// (either directly or though by other means of resolving a promise).


though by -> through || by ?

And maybe add promiseResolve to summary line 39?

const asyncHook = async_hooks.createHook( { init, before, after, destroy, promiseResolve } );

vsemozhetbyt · 2017-09-09T16:11:55Z

doc/api/async_hooks.md

+* `asyncId` {number}
+
+Called when the `resolve` function passed to the `Promise` constructor is
+invoked (either directly or though by other means of resolving a promise).


though by -> through || by ?

AndreasMadsen · 2017-09-09T16:21:56Z

I have a strange itch about this, are promises really the only case where we need this hook?

I spent a great deal of time thinking about it after #13437, I can't really come up with something good. Although, I suppose one could implement ones own promise in which case the Embedder API should be extended as well.

Ref: #13437

addaleax · 2017-09-09T16:28:58Z

@vsemozhetbyt Thanks, done!

I have a strange itch about this, are promises really the only case where we need this hook?

Me too! But I think so – the difference between this and other hooks is that Promises have their own ways of scheduling asynchronous work. I’d say resolve for Promises pretty much corresponds to before/after of other resources, but I don’t see a way to reflect that in the API.

(Except maybe setting up a separate async resource for the microtask queue, and then using before/after when the promise is resolved … ?)

JiaLiPassion · 2017-09-09T16:41:13Z

@addaleax , in this PR, the promiseResolveHook means resolve was called but not means promise was really resolved, is that right?

Just as we discussed in zone.js thread,

const p = new Promise((resolve, reject) => {}); // a promise will never resolve
const p1 = new Promise((resolve, reject) => { 
  resolve(p); //  p1 is not resolved, but PromiseResolve hook will be called?
});

is my understanding correct?
Is that possible to provide a hook to tell the promise is really resolved or rejected. (without add an additional Promise.then call)

Thank you very much!

addaleax · 2017-09-09T16:43:14Z

@JiaLiPassion Yes, your understanding is correct. I don’t quite like it myself, but this is what the V8 API offers. (cc @gsathya )

JiaLiPassion · 2017-09-09T16:45:56Z

@addaleax , got it, thank you!

jasnell · 2017-09-14T21:53:10Z

doc/api/async_hooks.md

+// promiseResolve is called only for promise resources, when the
+// `resolve` function passed to the `Promise` constructor is invoked
+// (either directly or through other means of resolving a promise).
+function promiseResolve(asyncId) { }


Would it be called before or after the resolve function is invoked? The distinction may be important for measuring timing.

@jasnell I’m pretty sure that’s actually irrelevant, since the resolve method doesn’t actually do any synchronously visible work.

This should also likely include some clarification about the relationship (if any) this has to the before and after emits. An example that shows what kind of ordering of these events to expect when doing a new Promise((resolve) => resolve(true)).then((a) => {}) would be worthwhile.

@addaleax

I’m pretty sure that’s actually irrelevant, since the resolve method doesn’t actually do any synchronously visible work.

Then I would be explicit about that in the docs.

I’ve added a note about that and added an example

trevnorris

I don't like adding another hook, but after thinking about this for quite a while I can't think of a better way to expose the interface. The only alternative I could think of is:

const hook = async_hooks.createHook({ /* ... */ }).enable();
hook.setPromiseResolve(cb);

i'd be partial to this approach if we thought there'd be more misc events like this in the future, but i can't think of any. @addaleax you think more events like this could occur in the future?

So, LGTM if there's no additional conversation on how the API is publicly exposed.

jasnell · 2017-09-14T23:41:36Z

I'm +1 on this approach also but we'll definitely need to do some testing on how this works with things like async iterators... just so we know what to expect. I don't anticipate any problems but it would be good to have a test for it. I'll do some testing on that shortly.

Looking at the various pending TC39 proposals I do not anticipate anything new and exotic that cannot be covered by the existing callbacks but it's difficult to say for sure that there wouldn't be. The Promise.prototype.finally proposal, for instance, should be covered by before/after. The only possible exception I can think of would be Promise cancelation, but that's difficult to say for sure because the proposal is far from being fleshed out.

jasnell · 2017-09-14T23:56:34Z

Quick test run with async iterators using the following:

The code...

'use strict';

const crypto = require('crypto');
const ah = require('async_hooks');

const hook = ah.createHook({
  init(id, type, triggerAsyncId, resource) {
    process._rawDebug(`${type} - ${id}, trigger: ${triggerAsyncId}`);
  },
  before(id) {
    process._rawDebug(`  before ${id}`);
  },
  after(id) {
    process._rawDebug(`  after ${id}`);
  },
  destroy(id) {
    process._rawDebug(`  destroy ${id}`);
  },
  promiseResolve(id) {
    process._rawDebug(`  promise resolve ${id}`);
  }
});

hook.enable();

async function hash(alg, data, enc) {
  const hash = crypto.createHash(alg);
  for await (const chunk of data) {
    hash.update(chunk);
  }
  return hash.digest(enc);
}

var n = 0;
async function* dataIterator() {
  while (n++ < 10)
    yield `testing${n}`;
}

const p = hash('sha256', dataIterator(), 'hex');
p.then(console.log).catch(console.error);

console.log('testing 123');

Here's the output

PROMISE - 5, trigger: 1                                          
PROMISE - 6, trigger: 1                                          
PROMISE - 7, trigger: 1                                          
  promise resolve 7                                              
PROMISE - 8, trigger: 5                                          
PROMISE - 9, trigger: 8                                          
  promise resolve 8                                              
PROMISE - 10, trigger: 5                                         
PROMISE - 11, trigger: 10                                        
testing 123                                                      
TickObject - 12, trigger: 1                                      
  before 12                                                      
  after 12                                                       
  before 6                                                       
  promise resolve 6                                              
  after 6                                                        
PROMISE - 13, trigger: 6                                         
  before 13                                                      
  promise resolve 8                                              
  promise resolve 13                                             
  after 13                                                       
  before 9                                                       
PROMISE - 14, trigger: 9                                         
PROMISE - 15, trigger: 9                                         
  promise resolve 15                                             
PROMISE - 16, trigger: 5                                         
PROMISE - 17, trigger: 16                                        
  promise resolve 16                                             
  promise resolve 9                                              
  after 9                                                        
  before 14                                                      
  promise resolve 14                                             
  after 14                                                       
PROMISE - 18, trigger: 14                                        
  before 18                                                      
  promise resolve 16                                             
  promise resolve 18                                             
  after 18                                                       
  before 17                                                      
PROMISE - 19, trigger: 17                                        
PROMISE - 20, trigger: 17                                        
  promise resolve 20                                             
PROMISE - 21, trigger: 5                                         
PROMISE - 22, trigger: 21                                        
  promise resolve 21                                             
  promise resolve 17                                             
  after 17                                                       
  before 19                                                      
  promise resolve 19                                             
  after 19                                                       
PROMISE - 23, trigger: 19                                        
  before 23                                                      
  promise resolve 21                                             
  promise resolve 23                                             
  after 23                                                       
  before 22                                                      
PROMISE - 24, trigger: 22                                        
PROMISE - 25, trigger: 22                                        
  promise resolve 25                                             
PROMISE - 26, trigger: 5                                         
PROMISE - 27, trigger: 26                                        
  promise resolve 26                                             
  promise resolve 22                                             
  after 22                                                       
  before 24                                                      
  promise resolve 24                                             
  after 24                                                       
PROMISE - 28, trigger: 24                                        
  before 28                                                      
  promise resolve 26                                             
  promise resolve 28                                             
  after 28                                                       
  before 27                                                      
PROMISE - 29, trigger: 27                                        
PROMISE - 30, trigger: 27                                        
  promise resolve 30                                             
PROMISE - 31, trigger: 5                                         
PROMISE - 32, trigger: 31                                        
  promise resolve 31                                             
  promise resolve 27                                             
  after 27                                                       
  before 29                                                      
  promise resolve 29                                             
  after 29                                                       
PROMISE - 33, trigger: 29                                        
  before 33                                                      
  promise resolve 31                                             
  promise resolve 33                                             
  after 33                                                       
  before 32                                                      
PROMISE - 34, trigger: 32                                        
PROMISE - 35, trigger: 32                                        
  promise resolve 35                                             
PROMISE - 36, trigger: 5                                         
PROMISE - 37, trigger: 36                                        
  promise resolve 36                                             
  promise resolve 32                                             
  after 32                                                       
  before 34                                                      
  promise resolve 34                                             
  after 34                                                       
PROMISE - 38, trigger: 34                                        
  before 38                                                      
  promise resolve 36                                             
  promise resolve 38                                             
  after 38                                                       
  before 37                                                      
PROMISE - 39, trigger: 37                                        
PROMISE - 40, trigger: 37                                        
  promise resolve 40                                             
PROMISE - 41, trigger: 5                                         
PROMISE - 42, trigger: 41                                        
  promise resolve 41                                             
  promise resolve 37                                             
  after 37                                                       
  before 39                                                      
  promise resolve 39                                             
  after 39                                                       
PROMISE - 43, trigger: 39                                        
  before 43                                                      
  promise resolve 41                                             
  promise resolve 43                                             
  after 43                                                       
  before 42                                                      
PROMISE - 44, trigger: 42                                        
PROMISE - 45, trigger: 42                                        
  promise resolve 45                                             
PROMISE - 46, trigger: 5                                         
PROMISE - 47, trigger: 46                                        
  promise resolve 46                                             
  promise resolve 42                                             
  after 42                                                       
  before 44                                                      
  promise resolve 44                                             
  after 44                                                       
PROMISE - 48, trigger: 44                                        
  before 48                                                      
  promise resolve 46                                             
  promise resolve 48                                             
  after 48                                                       
  before 47                                                      
PROMISE - 49, trigger: 47                                        
PROMISE - 50, trigger: 47                                        
  promise resolve 50                                             
PROMISE - 51, trigger: 5                                         
PROMISE - 52, trigger: 51                                        
  promise resolve 51                                             
  promise resolve 47                                             
  after 47                                                       
  before 49                                                      
  promise resolve 49                                             
  after 49                                                       
PROMISE - 53, trigger: 49                                        
  before 53                                                      
  promise resolve 51                                             
  promise resolve 53                                             
  after 53                                                       
  before 52                                                      
PROMISE - 54, trigger: 52                                        
PROMISE - 55, trigger: 52                                        
  promise resolve 55                                             
PROMISE - 56, trigger: 5                                         
PROMISE - 57, trigger: 56                                        
  promise resolve 56                                             
  promise resolve 52                                             
  after 52                                                       
  before 54                                                      
  promise resolve 54                                             
  after 54                                                       
PROMISE - 58, trigger: 54                                        
  before 58                                                      
  promise resolve 56                                             
  promise resolve 58                                             
  after 58                                                       
  before 57                                                      
PROMISE - 59, trigger: 57                                        
PROMISE - 60, trigger: 57                                        
  promise resolve 60                                             
PROMISE - 61, trigger: 5                                         
PROMISE - 62, trigger: 61                                        
  promise resolve 61                                             
  promise resolve 57                                             
  after 57                                                       
  before 59                                                      
  promise resolve 59                                             
  after 59                                                       
PROMISE - 63, trigger: 59                                        
  before 63                                                      
  promise resolve 61                                             
  promise resolve 63                                             
  after 63                                                       
  before 62                                                      
  promise resolve 5                                              
  promise resolve 62                                             
  after 62                                                       
  before 10                                                      
2a06f0027427dacc9b61df060cf9f413f6175ce9e3ba46bbf744a4132acb8d4f 
TickObject - 64, trigger: 10                                     
  promise resolve 10                                             
  after 10                                                       
  before 11                                                      
  promise resolve 11                                             
  after 11                                                       
  before 64                                                      
  after 64                                                       
  destroy 12                                                     
  destroy 64                                                     
james@ubuntu:~/node/node$

The only interesting bit that I can see here is that promise resolve is called twice in many instances. That's likely something worth looking into further.

Add a `promiseResolve()` hook.

addaleax · 2017-09-15T09:05:49Z

The only possible exception I can think of would be Promise cancelation, but that's difficult to say for sure because the proposal is far from being fleshed out.

I think we’re good with that, we can just use the destroy() hook.

The only interesting bit that I can see here is that promise resolve is called twice in many instances. That's likely something worth looking into further.

That seems to be expected to me in case where resolving one promise also resolves another?

AndreasMadsen · 2017-09-15T09:48:24Z

That seems to be expected to me in case where resolving one promise also resolves another?

Hmm, surely a promise can only be resolved once. In the example promise 21 is resolved twice (promise resolve 21).

addaleax · 2017-09-15T09:52:53Z

Oh. Yes, that is weird.

jasnell · 2017-09-15T10:49:23Z

The destroy hook would most likely be ok but cancel operations may end up having their own handler callback that invokes before the promise is actually destroyed.

And yeah, it's quite weird to have the promise reported as having been resolved twice. I don't think it's actually resolving twice. It's likely some weird side effect of the async iterator itself. I'll see if I can isolate what is causing it.

jasnell · 2017-09-15T17:34:15Z

The double resolve call is rather odd... ping @nodejs/v8 @ofrobots @fhinkel ... it would appear that when using an async iterator, the PromiseHook is being notified of the resolve more than once. I'll be investigating further but can anyone shed some light on whether this is expected behavior?

addaleax · 2017-09-15T17:54:23Z

Looks like Runtime_PromiseHookResolve() is actually called twice, so this is probably a V8 bug.

jasnell · 2017-09-15T17:57:35Z

That's what it's looking like from what I can see. It appears to have something to do with the way the async iterator's promise is resolved.

jasnell · 2017-09-15T17:59:43Z

That, btw, should not block this since async iterators aren't enabled by default yet. I've been unable to recreate the issue any other way.

matthewloring · 2017-09-15T18:01:17Z

@gsathya May have thoughts on where these multiple resolutions are coming from.

trevnorris · 2017-09-15T21:17:07Z

@addaleax @jasnell So there are potentially future promise events that wouldn't be covered by this and the existing 4 events? Brain dumping on this, but what about something like async_hooks.promises or async_hooks.promiseEvents that has all promise related events that can be taped into? this approach would allow taping into other async-ish events that aren't covered by either.

jasnell · 2017-09-15T21:45:45Z

There potentially are but it's just as likely that there won't be, which is what makes it difficult. Really the only possibility I can reasonably imagine right now would be a cancel hook that fires immediately before a destroy.

To be honest tho, I'd rather go with the approach of passing additional hooks on the original object the way this PR does.

BridgeAR · 2017-09-22T20:55:46Z

Where do we stand here? To me it looks like this could land as is? And is this actually semver-minor? I guess not because async_hooks is still experimental?

addaleax · 2017-09-22T21:24:30Z

@BridgeAR Yeah, I think this is ready, I appreciate the ping.

CI: https://ci.nodejs.org/job/node-test-commit/12537/

addaleax · 2017-09-23T13:58:47Z

Fixed linter error & landed in b605b15

Add a `promiseResolve()` hook. PR-URL: #15296 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add a `promiseResolve()` hook. PR-URL: nodejs/node#15296 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add a `promiseResolve()` hook. PR-URL: #15296 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #26978 Refs: #15296 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: #26978 Refs: #15296 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>

addaleax added async_hooks Issues and PRs related to the async hooks subsystem. promises Issues and PRs related to ECMAScript promises. labels Sep 9, 2017

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 Sep 9, 2017

addaleax removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 9, 2017

vsemozhetbyt reviewed Sep 9, 2017

View reviewed changes

jasnell reviewed Sep 14, 2017

View reviewed changes

trevnorris approved these changes Sep 14, 2017

View reviewed changes

jasnell approved these changes Sep 14, 2017

View reviewed changes

addaleax force-pushed the async-hooks-resolve branch from d6abb3c to d2d5d26 Compare September 15, 2017 08:59

async_hooks: support promise resolve hook

15befe1

Add a `promiseResolve()` hook.

addaleax force-pushed the async-hooks-resolve branch from d2d5d26 to 15befe1 Compare September 15, 2017 09:03

addaleax closed this Sep 23, 2017

addaleax deleted the async-hooks-resolve branch September 23, 2017 13:58

addaleax added a commit that referenced this pull request Sep 23, 2017

async_hooks: support promise resolve hook

b605b15

Add a `promiseResolve()` hook. PR-URL: #15296 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

refack mentioned this pull request Sep 23, 2017

async_hooks does not currently provide a way to track causality for a PromiseReactionJob #13437

Closed

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 23, 2017

async_hooks: support promise resolve hook

9e4f85c

Add a `promiseResolve()` hook. PR-URL: nodejs/node#15296 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell pushed a commit that referenced this pull request Sep 25, 2017

async_hooks: support promise resolve hook

17d8dfe

Add a `promiseResolve()` hook. PR-URL: #15296 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

akdor1154 mentioned this pull request Oct 13, 2017

@types/node: Asynchooks promiseResolve and AsyncResource DefinitelyTyped/DefinitelyTyped#20540

Merged

8 tasks

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

watson mentioned this pull request Mar 29, 2019

doc: list when promiseResolve hook was added to async_hooks #26978

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async_hooks: support promise resolve hook #15296

async_hooks: support promise resolve hook #15296

addaleax commented Sep 9, 2017

vsemozhetbyt Sep 9, 2017

vsemozhetbyt Sep 9, 2017 •

edited

Loading

vsemozhetbyt Sep 9, 2017

AndreasMadsen commented Sep 9, 2017 •

edited

Loading

addaleax commented Sep 9, 2017

JiaLiPassion commented Sep 9, 2017

addaleax commented Sep 9, 2017 •

edited

Loading

JiaLiPassion commented Sep 9, 2017

jasnell Sep 14, 2017

addaleax Sep 14, 2017

jasnell Sep 14, 2017

jasnell Sep 14, 2017

addaleax Sep 15, 2017

trevnorris left a comment

jasnell commented Sep 14, 2017

jasnell commented Sep 14, 2017

addaleax commented Sep 15, 2017

AndreasMadsen commented Sep 15, 2017

addaleax commented Sep 15, 2017

jasnell commented Sep 15, 2017

jasnell commented Sep 15, 2017

addaleax commented Sep 15, 2017

jasnell commented Sep 15, 2017

jasnell commented Sep 15, 2017

matthewloring commented Sep 15, 2017

trevnorris commented Sep 15, 2017 •

edited

Loading

jasnell commented Sep 15, 2017

BridgeAR commented Sep 22, 2017

addaleax commented Sep 22, 2017

addaleax commented Sep 23, 2017

async_hooks: support promise resolve hook #15296

async_hooks: support promise resolve hook #15296

Conversation

addaleax commented Sep 9, 2017

Checklist

Affected core subsystem(s)

vsemozhetbyt Sep 9, 2017

Choose a reason for hiding this comment

vsemozhetbyt Sep 9, 2017 • edited Loading

Choose a reason for hiding this comment

vsemozhetbyt Sep 9, 2017

Choose a reason for hiding this comment

AndreasMadsen commented Sep 9, 2017 • edited Loading

addaleax commented Sep 9, 2017

JiaLiPassion commented Sep 9, 2017

addaleax commented Sep 9, 2017 • edited Loading

JiaLiPassion commented Sep 9, 2017

jasnell Sep 14, 2017

Choose a reason for hiding this comment

addaleax Sep 14, 2017

Choose a reason for hiding this comment

jasnell Sep 14, 2017

Choose a reason for hiding this comment

jasnell Sep 14, 2017

Choose a reason for hiding this comment

addaleax Sep 15, 2017

Choose a reason for hiding this comment

trevnorris left a comment

Choose a reason for hiding this comment

jasnell commented Sep 14, 2017

jasnell commented Sep 14, 2017

addaleax commented Sep 15, 2017

AndreasMadsen commented Sep 15, 2017

addaleax commented Sep 15, 2017

jasnell commented Sep 15, 2017

jasnell commented Sep 15, 2017

addaleax commented Sep 15, 2017

jasnell commented Sep 15, 2017

jasnell commented Sep 15, 2017

matthewloring commented Sep 15, 2017

trevnorris commented Sep 15, 2017 • edited Loading

jasnell commented Sep 15, 2017

BridgeAR commented Sep 22, 2017

addaleax commented Sep 22, 2017

addaleax commented Sep 23, 2017

vsemozhetbyt Sep 9, 2017 •

edited

Loading

AndreasMadsen commented Sep 9, 2017 •

edited

Loading

addaleax commented Sep 9, 2017 •

edited

Loading

trevnorris commented Sep 15, 2017 •

edited

Loading