AsyncWrap for LTS Argon

[rvagg]

Topic for discussion, potential WG meeting agenda item. Coming out of nodejs/node#3461 /cc @thealphanerd, @trevnorris.

• AsyncWrap is an undocumented feature that we've allowed to break and evolve prior to being documented and made properly "public"
• It's a useful feature and may already be in use in user-land
• When it becomes more widely used in v5.x and beyond, the lack of parity with v4.x will be a big problem if we don't keep it updated
• If we go all the way and keep parity and even add documentation then it's likely a semver-minor, is that OK for LTS or is there a half-way we can pull off to keep it at parity but technically not a feature addition (like not adding documentaiton)?

A quick report from @trevnorris on its status and how close to final(ish) it is would be helpful here.

[MylesBorins]

In the past we have been able to get stats and how often an api is used in user land, I think it was @orangemocha or @ChALkeR

If we can get a list of modules that rely on the undocumented API we could run citgm against them

[rvagg]

Sorry, just to be clear—AsyncWrap is already in v4.x but as @trevnorris has been improving it the API has been shifting and breaking, we just haven't been labeling with semver-minor/major for any of it yet. The comment about parity with v5.x/master is that it's already diverged (i.e. nodejs/node#3461) and may continue to do so until final. We could end up in a state where it's a different API on v4.x than master.

[trevnorris]

The API is feeling pretty good to me. There are a couple things that could be done to make it more future proof (e.g. pass an object to setupHooks() instead of functions), but other than that I think future enhancements can be made without semver-major API changes. That is, if what's currently in master plus those few additional changes mentioned here land in v4.

Now, there are two big features that people have wanted, but haven't proceeded with because don't want to spend the time writing these again if they won't be accepted. Those are:

• Timers. Currently we track TimerWrap, but since we aggregate timers on a single instance this doesn't give us proper stacks. I've done it before and I know others would like to see it in there, but was removed the first time for being too invasive.
• async error handling. I was able to reproduce almost all of domain's capabilities aside from one edge case, but this involved hacking the JS API a little because of some discrepancies between when event occur. e.g. net.createServer() creates the JS stream but it's not until .listen() that the native class is instantiated.

These should be possible to do without breaking the API, but also are completely additive. I write these two as an example of what may be coming, and how it may affect LTS.

[AndreasMadsen]

/cc @nodejs/tracing

[AndreasMadsen]

@thealphanerd there is a list of all process.binding uses in nodejs/node#2768 (https://gist.github.com/ChALkeR/2e308b9f888456bfa4b0). At that point in time (26 Sep 2015) there was only two modules that used async_wrap:

dprof-0.10.1.tgz/async_wrap.js:3:var asyncWrap = process.binding('async_wrap');
trace-1.2.0.tgz/tracing_polyfill.js:6:var asyncWrap = process.binding('async_wrap');

Both of those are mine and I would much prefer getting breaking updates, than no updates.

@trevnorris If you don't think any of the handle context issues listed in nodejs/diagnostics#29 will be API breaking, then I'm okay with freezing the API in node 4.x, assuming we will still get fixes.

But I would like to see a clear strategy for how native modules should interface with AsyncWrap. I realize that the AsyncWrap constructor is now public, but it is not clear for me how that works and I don't think it fits into the current NAN paradigme.

Fixing timers, would as such be breaking. I'm currently monkey-patching the timers functions and ignorering the Timer handle in async_wrap. I don't think that strategy will continue to work, once the timer issue is fixed.

[trevnorris]

@AndreasMadsen Making AsyncWrap public doesn't do anything for a native interface. I see the best option to create a public virtual class that the user needs to implement. Similar to v8::ArrayBuffer::Allocator. Something like:

class AsyncWrap : public ObjectWrap {
  class Callbacks {
   public:
    virtual ~Callbacks() {}
    virtual void Init(Local<Object> context, int64_t id, int provider, Local<Object> parent) = 0;
    virtual void Pre(Local<Object> context) {}
    virtual void Post(Local<Object> context) {}
    virtual void Destruct(int64_t id) {}
    AsyncWrap* Instance();
  };

  void SetCallbackHandler(Callbacks* cb);

This will allow the user to receive the same callbacks that JS receives. Thoughts?

[ChALkeR]

@thealphanerd
lzgrep -h binding.*async_wrap slim.code.coffee.lzo slim.code.js.lzo slim.code.ts.lzo:

dprof-0.10.1.tgz/async_wrap.js:3:var asyncWrap = process.binding('async_wrap');
trace-1.2.0.tgz/tracing_polyfill.js:6:var asyncWrap = process.binding('async_wrap');

The data is more than two months old, based only on latest versions (at 2015-09-21) of npm packages and there could be false negatives.

I will update the dataset, but a bit later (not this week).

[MylesBorins]

So it would seem to me like there should be no immediate or obvious problem. Since 4.x has another 29 month's to go I think we would all benefit quite a bit from keeping apis as consistent as possible

[AndreasMadsen]

@trevnorris I'm not sure we are talking about the same thing, I'm talking about an replacement for the _asyncQueue hack. A native setupHooks interface would also be nice and perhaps very useful for trace_event.h, but I don't consider it essential myself.

[AndreasMadsen]

Another problem with async_wrap is that only one setupHooks call is allowed, another call will just overwrite the previous hooks. This means that modules will conflict with each other.

Right now very few people knows about async_wrap so it isn't an issue, but once it goes public it will become a big issue.

[AndreasMadsen]

For the LTS group, this is the current issue tracker: nodejs/diagnostics#29

[dshaw]

I think adding documentation to LTS would be a step too far.

[trevnorris]

@AndreasMadsen Ah yes, in that case the public AsyncWrap class would do the trick. With the PR I'll add tests demonstrating how this is to be used, and can suppliant with additional documentation.

As far as multiple calls to setupHooks(). That's where I want to leave it. Allowing an arbitrary number of callbacks to be passed introduces a certain amount of complexity that I didn't want in core. One reason why I liked having it more hidden is because then users like yourself were able to instrument it in more complete ways. One of which could be to allow multiple callbacks to be passed.

[AndreasMadsen]

As far as multiple calls to setupHooks(). That's where I want to leave it.

@trevnorris That is a huge mistake and I speak from experience.

I love that node.js keeps things simple, but simplicity shouldn't be prioritized over modularity. Yes modularity can be done in userland, but it only works when there are 100% adoption. That will never happen for a userland module, that is why it needs to be in core.

I speak from experience because the same issue exists with Error. prepareStackTrace. Only one thing may set it, setting it twice will conflict. I wrote https://github.com/AndreasMadsen/stack-chain as a modular interface for Error.prepareStackTrace. It is used by quite a few modules:

cucumber, xo-server, cucumber-redbox, ramda-t, dprof, cucumber-sigma,
julien-f-source-map-support, trace, cucumber-for-jenkins, stack-json, ferror,
clarify, traceurified, hide-stack-frames-from

however it is impossible for me to get 100% adoption. Overwriting Error.prepareStackTrace simply makes people feel more powerful and it's more fun, or some just don't know better. Thus there are still important modules such as longjohn and coffee-script (particularly nasty) that continues to overwrite Error.prepareStackTrace. It creates hard to debug issues (AndreasMadsen/trace#9) and the workarounds are even worse (https://github.com/AndreasMadsen/stack-chain/blob/master/stack-chain.js#L135L160).

Having a single modular interface in core, will create a much better ecosystem. I think that is worth the price of some complexity in core.

I had hoped to do this discussion later, when async_wrap was more mature. But since the maturity of async_wrap is being discussed, I'm doing it now.

[trevnorris]

@AndreasMadsen But there's still a key different between what you've described and what happens with AsyncWrap callbacks. In all callback cases the object instance is set as the context. Any user can then manipulate the instance. There could equally be conflicting assignments that lead to undefined behavior. The difference is that this case is more cryptic to debug.

This is a topic I've entertained, and even prototyped, but every time I do I find it easy to clobber something unexpectedly. I'd like to bring this up with the CTC for discussion and get their feedback.

[trevnorris]

Discussion topic for @nodejs/ctc: Allow single set of hooks, or multiple sets of hooks to be passed to async_wrap.setupHooks() (which would probably be renamed in that case to addHooks()). @AndreasMadsen has outlined the pros above, focusing on creating a more unified interface that doesn't depend on other modules to use. While I do not disagree with this sentiment, my experience has shown me that by passing the same object instance to multiple users it opens the API up for more unexpected behavior by users doing things such as adding properties to the object instance of the same name.

[Jeff-Lewis]

As for user land stats, doesn't continuation-local-storage use it if --harmony flag is on? CLS had 122K+ downloads last month.

Also, Strongloop is contemplating deprecating their use of a Context (which I hope they don't), because of the current reliability issues related to what hopefully an improved async_wrap with solve. Loopback uses CLS as well.

I'm dying for a reliable async_wrap and context across asyc boundaries. To me, it's one of (the few) weaknesses of Node compared to other more established server-side run-times.

[ofrobots]

doesn't continuation-local-storage use it if --harmony flag is on

AFAIK, cls uses async-listener, not async-wrap.

current reliability issues related to what hopefully an improved async_wrap with solve

This is a digression, but I don't think async_wrap can solve the user-space queuing issues that break continuation-local-storage.

[Jeff-Lewis]

Sorry for the digression. Isn't AsyncListener an API for AsyncWrap?

[trevnorris]

I worked with NR developing AsyncListener. In the end the entire user interface was removed (ref nodejs/node-v0.x-archive#8110). The decision was we didn't want to introduce another "domain" issue (also attempted nodejs/node-v0.x-archive#6011) where we tied ourselves into an API that we couldn't get out of. Currently NR uses a shim that emulates AsyncListener.

After sinking a good part of 3 months into it, stepped back from it and have only been incrementally working on it since then.

AFAIK, cls uses async-listener, not async-wrap.

Technically async-listener should be using async-warp.

[rvagg]

CTC discussion suggested that AsyncWrap should be brought up to parity with the final version, just not announced as a public and supported API in v4.