node-forward / discussions

Soliciting ideas and feedback for community driven collaborative projects that help Node.
149 stars 1 forks source link

Lessons learned from async listener #28

Open domenic opened 9 years ago

domenic commented 9 years ago

In my opinion, the lack of "async listener"-esque functionality is a bug in the JavaScript language. There should be something built in to the language, and the engine, that allows you to intercept any entries or exits from the event loop. Then, on top of that, you could build domains, async try/catch, long stack traces, etc.

Currently in browsers people do this by patching every async API. Node's async listener was, if I understand it, an attempt at a more principled approach. However, it was removed.

I'd love to understand the "lessons learned" from the work on async listener so far. What worked? What didn't? What were the problems? Was there a fatal flaw that prevented the API from shipping, or was it just an overall feeling that it wasn't quite right, or...? Hopefully we can take this information and work out something that could eventually be baked into the JS language itself, across all environments. Informed, of course, by careful prototypes by us people :).

/cc @trevnorris @piscisaureus @othiym23 @creationix @btford plus lots more I am presumably forgetting.

ijroth commented 9 years ago

@kraman @sam-github @bnoordhuis

Qard commented 9 years ago

This might be a useful read: https://gist.github.com/groundwater/942dad5c0c4cfae21af9

That is from a Tracing Summit meeting from several months ago. The general consensus seemed to be that AsyncListener was awkward and unreliable for our use case.

I'll gladly help in any way I can with defining an AsyncListener-like thing for JS. :)

othiym23 commented 9 years ago

The API was unwieldy and came in at an awkward layer of abstraction. It was perfect for continuation-local storage's purposes, kinda, but at (what we presumed was) the end of the 0.12 release cycle, it felt like the design of the API needed some more time to bake. The intention was always to land a more fully-baked tracing API before Node hit 1.0, and the notes that @groundwater pulled together are meant as a jumping-off point for future work. It's not a dead letter!

@domenic I think all of us who were at the meeting that @Qard linked to were in agreement that better access to instrumentation and operational reflection from the Node runtime would be hugely useful, but I'm not sure how well that meshes the concerns I see within TC39 regarding security and preventing inadvertent information disclosure. asyncListener presumes a trusted environment, and a lot of its power comes from it having visibility into parts of the execution cycle that are normally obscured by design in JS. But I would love to see better instrumentation available in JS, so I'll let TC39 types decide what is and isn't appropriate wrt those criteria.

creationix commented 9 years ago

I always wanted a simple API for node, long before we had domains or async-listner. The core idea is:

function onSetup(type, handle) {
  // Here I can do things like look at current globals to know what domain I'm in or what http request I'm tracing, etc.
  return {};
}
function onEvent(context, event) {
  // Do things before the event, like setting globals or tracing stuff
  event();
  // Do things after as well
}

This simple, but powerful API can do all sorts of crazy things. It could implement domains using try..catch..finally in the onEvent handler. It could silence certain types of events entirely, it could give http code globals like req and res and swap them out on every event source.

The main concern I've heard with this approach is it would kill performance. But after seeing the issues with the other approaches, I'm not convinced performance is so killer in such a simple API.

I had this interface in luvit 1.0 and it was still considerably faster than node.js. But that may be because luajit has much lower overhead switching between lua and C compared to V8 switching between JS and C++.

creationix commented 9 years ago

Btw, when I worked on async-listener, I quickly realized that backwards compat with the original domains was quite complex and treated event listeners as an async source when they were a purely sync action.

As others have noted, the real complexities are when you have multi emitters like intervals, tcp servers, and low-level streams. The libuv team has ideas to unify this all into single event type APIs (more like requestAnimationFrame and less like setInterval). If all event sources APIs were converted to this style, even the connection pooling problem would be less of an issue (though still needs app-level intervention for multiplexed responses over a single channel).

domenic commented 9 years ago

One thing that seems clear to me is that we want to focus on async entry points, and not special-case EventEmitter at all. If code enters the event loop by firing an event, that hits some hook, just like if code enters the event loop by calling a callback. (In both cases, "C++ is calling JS.") But if JS code fires an event itself, that doesn't hit the hook.

bjouhier commented 9 years ago

There are several drivers behind these hooks. I see two that really justify the hooks:

But we don't need additional APIs for exception handling or long stack traces. These issues will be solved by the async/await + promises combination in ES7 and if people want them today, they can use one of the libraries/tools that provide async/await emulation. The good ones support structured EH with long stack traces.

Ideally, I think that Local Storage should also be handed at the language level rather than through special APIs. This is important for interop between libraries. For example, it would be good to have some kind of TLS locale as part of the Intl library.

Otherwise, +1 on @domenic's comment that the focus should be on async entry points rather than on event emitters. BTW, this is what I have put in streamline.js: a pair of yield/resume events that fire when code enters/exits the event loop, with a context that is preserved across await and accessible from these events. If you can capture the stack (long one of course) at the yielding point (the await call that caused the yield), then you have a very powerful instrumentation hook, and for example you can capture flamegraphs of async calls (see https://github.com/Sage/streamline-flamegraph).

trevnorris commented 9 years ago

I'll reply to this in more detail later, but want to make sure everyone understands that the hooks async listener used are still there. Technically async listener could ship as a module, which was the point. Didn't want to have another domains issue where we find out after the fact the API has issues. So I wanted the community to write their own implementations and come to a consensus of what the best API would be.

piscisaureus commented 9 years ago

I'll write up my experience with it too; right now I'm busy with the io.js release but that should be over next week.

kraman commented 9 years ago

I am working on an API that would meet the goals of async-listener as well as other APIs that require tracking context across the C boundary. You can take a look here https://github.com/strongloop/async-tracker.

Once implemented this API will provide events for core functions which should satisfy @domenic 's use case. It would also let non-core node programmers instrument their own code if they would like additional events tracked. It also handles one-show callbacks as well as repeat-callbacks in the same way.

I haven't thought about how the async/await style would affect this as yet and the implementation is still very inefficient but I am interested in learning if the API satisfies what is expect from async-listener like APIs.