implement domain-like hooks (asynclistener) for userland #6011

Closed
wants to merge 9 commits into
from

Projects

None yet
@trevnorris

Here's the current API planned for implementation:

// Class that will store information about a specific request.
function Storage() { }

// This will be called every time asynchronous work is queued.
// The returned data will propagate to the callbackObject's callbacks.
function onCreation() {
  return new Storage();
}

// Set of methods that will run at specific points in time according to
// their cooresponding callbacks. These methods are always run FIFO if
// multiple are queued.
// "context" is the "this" of the request object.
var callbackObject = {
  before: function asyncBefore(context, storageValue) {
  },
  // "returnValue" is the value returned from the callback.
  after: function asyncAfter(context, storageValue) {
  },
  // If this callback returns "true" then the error handler will assume
  // the error was properly handled, and process will continue normally.
  // If multiple error handlers are queued, and any one of those returns
  // true, then Node will assume the error was properly handled.
  // This will not currently be passed the context (or "this") of the
  // callback that threw. A simple way of achieving this is currently
  // being investigated, and the feature will be added when one is found.
  error: function asyncError(storageValue, err) {
  }
};

/**
 * process.addAsyncListener(callback[, object[, value]]);
 *
 * Arguments:
 *
 * callback - Function that will be run when an asynchronous job is
 *    queued.
 *
 * object - Object with the optional callbacks set on:
 *    before - Callback called just before the asynchronous callback
 *        will be called.
 *    after - Callback called directly after the asynchronous callback.
 *    error - Callback called if there was an error.
 *
 * The returned key is an Object that serves as the unique key for the
 * call (much like how Timers work).
 */
var key = process.addAsyncListener(onAsync, callbackObject);


/**
 * process.createAsyncListener(callback[, object[, value]]);
 *
 * Adding an async listener will immediately add it to the queue and
 * being listening for events. If you wish to create the listener in
 * advance, to say attach to the returned value object, it's possible
 * to get the key and pass it to process.addAsyncListener() later.
 */
var key = process.createAsyncListener(onAsync, callbackObject, value);

// Then activate like so:
process.addAsyncListener(key);


/**
 * process.removeAsyncListener(key);
 *
 * Remove any async listeners and associated callbackObjects. All
 * listeners will live independent of each other, and there will be no
 * method given to clear all async listeners.
 */
process.removeAsyncListener(key);
@othiym23
othiym23 commented Aug 7, 2013

This would meet my requirements, as long as the async listener also fires for the timer functions and is invoked from Make(Domain)Callback. Also, I'm pretty sure that if you're going to expose a generic hook like this, it's going to need the ability to accept multiple listeners. If those are pieces you plan to add to this, 👍 from me.

@trevnorris

@othiym23 thanks for the feedback. that was along the lines of what I was attempting. exposing multiple listeners is easy. already have a patch for it, but removed it when I realized that users could easily wrap callbacks in callbacks in callbacks. and there's no guaranteed order of firing. also, do I only allow the same listener to be added once, or can they add multiple? it was getting complicated fast so it was dropped so I could get something solid posted.

i'll happily add it back if there's a clear road of where it should go.

@othiym23
othiym23 commented Aug 7, 2013

I think the most sensible, intuitive way for this to work would be to run the listeners in the order that they were registered via addAsyncListener, with each listener receiving the callback returned by the previous listener. It's up for the writers of the listeners to ensure that they're safe in how they create the closures. Regardless, the order in which they're run must be deterministic and stable (i.e. using an array for storage, not an object).

It's hard to avoid some difficulty with ordering dependencies, though. It might be enough to offer two calls to add a listener -- insertAsyncListener and addAsyncListener. If that's insufficient, giving modules that register listeners the power to dump and reorder the list of listeners might be necessary. I can't imagine a situation in which you'd want to have more than a couple of these listeners at a time (oh, the performance costs!), but you're right that there's an important semantic distinction to draw between outer and inner wrappers.

I don't really have it clear in my head, but it seems like it could be possible to use that object you were talking about yesterday as a means for these wrappers to share their state, and maybe externalize whatever data might otherwise subject to dependencies. But down that road lies a whole bunch of other annoying complications and complexity.

@creationix

I just sent pull requests to CLS to use this API and to CLS-Glue to prollyfill this API.

@trevnorris did I implement the proposed API correctly in the polyfill?

@creationix

@othiym23 Also, once/if this lands in core and CLS becomes peer to Domains instead of domains being implemented on top of CLS, I recommend simplifying the CLS API and implementation to be just data storage. https://gist.github.com/creationix/d531157ad587a4af9f4e#file-multi-cls-js

@othiym23
othiym23 commented Sep 2, 2013

@creationix: I went ahead and split your proposed changes out into an entirely separate module, async-listener. I then went and included that polyfill directly in continuation-local-storage, putting the relevant tests in relevant places. I pulled Trevor's tests from this PR, and a few of his test cases are currently failing, but I'll bang on that some more tomorrow.

@trevnorris, what this means is that we now have a polyfill for versions of node < (whatever version of Node this lands in). I'll keep it up to date with your changes, and you can grab a copy of continuation-local-storage and run its tests against your fork if you want another source of test coverage.

@trevnorris

@othiym23 Thanks. Trying to have basics working by NodeConf.eu, but working around everything already in place has been a little painful. To be honest one problem is that I'm catching more asynchronous callbacks then expected. Which make the tests unpredictable. So I need to trace down where every one is occurring so they can be accounted for.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Sep 7, 2013
lib/timers.js
immediate._onImmediate();
- if (domain) domain.exit();
+
+ if (domain)
+ domain.exit();
@bnoordhuis
bnoordhuis Sep 7, 2013 Member

I thought domains were going to be implemented in terms of async listeners?

@trevnorris
trevnorris Sep 7, 2013

Still in the middle of figuring that out. Domains are so tightly coupled
with the EventEmitter that it's been a little difficult.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Sep 7, 2013
src/async_wrap.h
+#define SRC_ASYNC_WRAP_H_
+
+#include "env.h"
+#include "v8.h"
+#include <assert.h>
+
+namespace node {
+
+class AsyncWrap {
+ public:
+ AsyncWrap(Environment* env, v8::Handle<v8::Object> object)
+ : async_flags_(0) {
+ if (object.IsEmpty())
+ return;
+
+ if (env->active_async_listeners()) {
@bnoordhuis
bnoordhuis Sep 7, 2013 Member

Can non-active listeners exist? If not, then this function name is misleading.

@trevnorris
trevnorris Sep 16, 2013

it was meant more that there are async listeners actively listening, but also there can be async listeners pushed back in the listening queue that aren't active.

@bnoordhuis bnoordhuis commented on an outdated diff Sep 7, 2013
src/async_wrap.h
+#include "env.h"
+#include "v8.h"
+#include <assert.h>
+
+namespace node {
+
+class AsyncWrap {
+ public:
+ AsyncWrap(Environment* env, v8::Handle<v8::Object> object)
+ : async_flags_(0) {
+ if (object.IsEmpty())
+ return;
+
+ if (env->active_async_listeners()) {
+ v8::Local<v8::Value> arg = object.As<v8::Value>();
+ env->async_listener_function()->Call(object, 1, &arg);
@bnoordhuis
bnoordhuis Sep 7, 2013 Member

What if the function throws an exception?

@bnoordhuis
bnoordhuis Sep 7, 2013 Member

Also, is the assumption here that there is a Context::Scope somewhere on the stack? If so, you should probably assert that:

assert(v8::Context::GetCurrent() == object->GetCreationContext());

We should probably add Google-style ASSERT() and CHECK() macros because that's something you probably only want to check in debug builds. (Not that it's hugely expensive but still.)

@bnoordhuis bnoordhuis commented on an outdated diff Sep 7, 2013
src/async_wrap.h
+ v8::Local<v8::Value> arg = object.As<v8::Value>();
+ env->async_listener_function()->Call(object, 1, &arg);
+ async_flags_ |= ASYNC_LISTENERS;
+ }
+ }
+
+ inline uint32_t async_flags() const {
+ return async_flags_;
+ }
+
+ static inline bool has_async_listeners(uint32_t flag) {
+ return flag & ASYNC_LISTENERS;
+ }
+
+ private:
+ static enum AsyncFlags {
@bnoordhuis
bnoordhuis Sep 7, 2013 Member

static makes no sense here.

EDIT: Or rather, it does in the sense that you're declaring both the AsyncFlags enum here and a static member variable called AsyncFlags. Probably not what you intended.

@bnoordhuis bnoordhuis commented on an outdated diff Sep 7, 2013
src/async_wrap.h
+ }
+
+ private:
+ static enum AsyncFlags {
+ NO_OPTIONS = 0,
+ ASYNC_LISTENERS = 1
+ } AsyncFlags;
+
+ uint32_t async_flags_;
+};
+
+
+} // namespace node
+
+
+#endif // SRC_ASYNC_WRAP_H_
@bnoordhuis
bnoordhuis Sep 7, 2013 Member

Please split this file in two: an async-wrap.h that contains just the definition and an async-wrap-inl.h that contains the implementation. I intend to do the same thing with req_wrap.h and friends in the near future.

@bnoordhuis bnoordhuis commented on an outdated diff Sep 7, 2013
src/env-inl.h
inline Environment::TickInfo::TickInfo() {
for (int i = 0; i < kFieldsCount; ++i) fields_[i] = 0;
+ in_tick_ = false;
+ last_threw_ = false;
@bnoordhuis
bnoordhuis Sep 7, 2013 Member
inline Environment::TickInfo::TickInfo() : in_tick_(false), last_threw_(false) {
@bnoordhuis bnoordhuis commented on an outdated diff Sep 7, 2013
src/env.h
@@ -58,6 +58,7 @@
V(PUT_string, "PUT") \
V(address_string, "address") \
V(atime_string, "atime") \
+ V(asyncqueue_string, "_asyncQueue") \
@bnoordhuis
bnoordhuis Sep 7, 2013 Member

async_queue_string?

@bnoordhuis bnoordhuis commented on an outdated diff Sep 7, 2013
src/fs_event_wrap.cc
@@ -171,7 +171,8 @@ void FSEventWrap::OnEvent(uv_fs_event_t* handle, const char* filename,
wrap->object(),
env->onchange_string(),
ARRAY_SIZE(argv),
- argv);
+ argv,
+ wrap->async_flags());
@bnoordhuis
bnoordhuis Sep 7, 2013 Member

Rather than patching up MakeCallback everywhere, it might be a good idea to add a AsyncWrap::MakeCallback() method that does the right thing. IOW:

wrap->MakeCallback(env->onchange_string(), ARRAY_SIZE(argv), argv);

(I'm making the assumption here that the AsyncWrap already knows what object to use.)

@bnoordhuis bnoordhuis commented on an outdated diff Sep 7, 2013
src/node.cc
@@ -878,6 +879,35 @@ void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
}
+void SetupAsyncListener(const FunctionCallbackInfo<Value>& args) {
+ Environment* env = Environment::GetCurrent(node_isolate);
+ HandleScope scope(node_isolate);
@bnoordhuis
bnoordhuis Sep 7, 2013 Member

It's not critical but before landing the multi-context patch I rewrote it to use the following everywhere:

Environment* env = Environment::GetCurrent(args.GetIsolate());
HandleScope handle_scope(args.GetIsolate());

(The handle_scope is to make it clear it's a HandleScope and not a Context::Scope.)

@bnoordhuis bnoordhuis commented on an outdated diff Sep 7, 2013
src/node.cc
@@ -878,6 +879,35 @@ void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
}
+void SetupAsyncListener(const FunctionCallbackInfo<Value>& args) {
+ Environment* env = Environment::GetCurrent(node_isolate);
+ HandleScope scope(node_isolate);
+
+ if (!args[0]->IsObject() ||
+ !args[1]->IsFunction() ||
+ !args[2]->IsFunction() ||
+ !args[3]->IsFunction()) {
+ fprintf(stderr, "_setupAsyncListener arguments are incorrect types");
+ abort();
+ }
@bnoordhuis
bnoordhuis Sep 7, 2013 Member

Just assert? This fprintf() + abort() style is kind of weird. (I know e.g. NODE_UNWRAP() does the same thing. Also kind of weird IMO.)

@bnoordhuis bnoordhuis commented on an outdated diff Sep 7, 2013
src/node.cc
+ Environment* env = Environment::GetCurrent(node_isolate);
+ HandleScope scope(node_isolate);
+
+ if (!args[0]->IsObject() ||
+ !args[1]->IsFunction() ||
+ !args[2]->IsFunction() ||
+ !args[3]->IsFunction()) {
+ fprintf(stderr, "_setupAsyncListener arguments are incorrect types");
+ abort();
+ }
+ env->set_async_listener_function(args[1].As<Function>());
+ env->set_async_listener_load_function(args[2].As<Function>());
+ env->set_async_listener_unload_function(args[3].As<Function>());
+
+ Local<Object> async_listener_flag_obj = args[0].As<Object>();
+ Environment::AsyncListener* async_listener_count =
@bnoordhuis
bnoordhuis Sep 7, 2013 Member

Peculiar variable name.

@bnoordhuis bnoordhuis commented on an outdated diff Sep 7, 2013
src/node.cc
+ HandleScope scope(node_isolate);
+
+ if (!args[0]->IsObject() ||
+ !args[1]->IsFunction() ||
+ !args[2]->IsFunction() ||
+ !args[3]->IsFunction()) {
+ fprintf(stderr, "_setupAsyncListener arguments are incorrect types");
+ abort();
+ }
+ env->set_async_listener_function(args[1].As<Function>());
+ env->set_async_listener_load_function(args[2].As<Function>());
+ env->set_async_listener_unload_function(args[3].As<Function>());
+
+ Local<Object> async_listener_flag_obj = args[0].As<Object>();
+ Environment::AsyncListener* async_listener_count =
+ env->async_listener_count();
@bnoordhuis
bnoordhuis Sep 7, 2013 Member

Or maybe it's a peculiar method name. :-)

@bnoordhuis bnoordhuis commented on an outdated diff Sep 30, 2013
src/env-inl.h
@@ -184,6 +204,11 @@ inline v8::Isolate* Environment::isolate() const {
return isolate_;
}
+inline bool Environment::active_async_listeners() const {
@bnoordhuis
bnoordhuis Sep 30, 2013 Member

has_async_listeners() is a better name for this, it spells out that the return value is a boolean.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Sep 30, 2013
src/node.cc
@@ -875,6 +904,32 @@ void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
domain_flag->fields(),
kExternalUnsignedIntArray,
domain_flag->fields_count());
+
+ // Do a little housekeeping.
+ env->process_object()->Delete(
+ FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupDomainUse"));
+}
+
+
+void SetupNextTick(const FunctionCallbackInfo<Value>& args) {
+ Environment* env = Environment::GetCurrent(args.GetIsolate());
+ HandleScope handle_scope(args.GetIsolate());
+
+ if (!args[0]->IsObject() || !args[1]->IsFunction())
@bnoordhuis bnoordhuis commented on an outdated diff Sep 30, 2013
src/node.cc
@@ -835,6 +837,33 @@ static const char *winapi_strerror(const int errorno) {
#endif
+void SetupAsyncListener(const FunctionCallbackInfo<Value>& args) {
+ Environment* env = Environment::GetCurrent(args.GetIsolate());
+ HandleScope handle_scope(args.GetIsolate());
+
+ if (!args[0]->IsObject() ||
+ !args[1]->IsFunction() ||
+ !args[2]->IsFunction() ||
+ !args[3]->IsFunction())
+ abort();
@bnoordhuis bnoordhuis commented on an outdated diff Sep 30, 2013
src/node_crypto.cc
argv[0] = Exception::Error(OneByteString(node_isolate, errmsg));
argv[1] = Null(node_isolate);
} else {
argv[0] = Null(node_isolate);
- argv[1] = Buffer::Use(req->data_, req->size_);
- req->data_ = NULL;
+ argv[1] = Buffer::Use(req->data(), req->size());
+ req->set_data(NULL);
@bnoordhuis
bnoordhuis Sep 30, 2013 Member

This is an awfully implicit way of transferring ownership. It's better to move this logic into the class itself.

Apart from that, I like how you cleaned up RandomBytesRequest.

@bnoordhuis bnoordhuis commented on an outdated diff Sep 30, 2013
src/node_crypto.cc
}
- free(req->data_);
+ free(req->data());
@bnoordhuis
bnoordhuis Sep 30, 2013 Member

This should be moved into the class if possible.

@othiym23 othiym23 commented on an outdated diff Oct 4, 2013
lib/domain.js
+ // One of three things will happen here.
+ //
+ // 1. There is a handler, caught = true
+ // 2. There is no handler, caught = false
+ // 3. It throws, caught = false
+ //
+ // If caught is false after this, then there's no need to exit()
+ // the domain, because we're going to crash the process anyway.
+ caught = domain.emit('error', er);
+
+ // Exit all domains on the stack. Uncaught exceptions end the
+ // current tick and no domains should be left on the stack
+ // between ticks.
+ stack.length = 0;
+ exports.active = process.domain = null;
+ process.removeAsyncListener(domain._listener);
@othiym23
othiym23 Oct 4, 2013

shouldn't this be in a finally clause below the next catch?

@othiym23 othiym23 commented on the diff Oct 4, 2013
lib/domain.js
Domain.prototype.run = function(fn) {
- return this.bind(fn)();
+ if (this._disposed)
+ return;
+ this.enter();
+ var ret = fn.call(this);
@othiym23
othiym23 Oct 4, 2013

I don't know about the JIT implications of this (my understanding is that single-call try clauses don't inhibit optimization, but I could be wrong), but I use this idiom in the asyncListener polyfill:

try {
  return fn.call(this);
}
finally {
  this.exit();
}
@trevnorris
trevnorris Oct 4, 2013

The listener is supposed to stay active if there's an error. That's how _fatalException() knows what error callbacks to call.

@isaacs
isaacs Oct 28, 2013

@trevnorris It could still work, but it would mean moving the domain.exit() logic out of _fatalException() and having it run in the finally block.

@isaacs
isaacs Oct 28, 2013

That is, if you have a throwing fn, the flow would be:

try block
enter domain
fn() up until the throw
!throw!
error handling logic (_fatalException, etc)
finally block
this.exit()

For a non-throwing fn, it'd be:

try block
enter domain
fn()
set return value
finally block
this.exit()
return
@othiym23 othiym23 commented on an outdated diff Oct 4, 2013
lib/events.js
- er.domainEmitter = this;
- er.domain = this.domain;
- er.domainThrown = false;
- this.domain.emit('error', er);
- } else if (er instanceof Error) {
- throw er; // Unhandled 'error' event
- } else {
- throw TypeError('Uncaught, unspecified "error" event.');
- }
- return false;
+ if (type === 'error' && !this._events.error) {
+ er = arguments[1];
+ if (this.domain) {
+ if (!er)
+ er = new TypeError('Uncaught, unspecified "error" event.');
+ er.domainEmitter = this;
@othiym23
othiym23 Oct 4, 2013

Maybe copy the logic for using util._extend used in domain.js?

@othiym23
othiym23 Oct 4, 2013

Or maybe replace the use of util._extend above? Seems like direct mutation would have to be faster than using a helper function.

@othiym23 othiym23 commented on the diff Oct 4, 2013
lib/timers.js
@@ -30,6 +30,17 @@ var TIMEOUT_MAX = 2147483647; // 2^31-1
var debug = require('util').debuglog('timer');
+var asyncFlags = process._asyncFlags;
+var runAsyncQueue = process._runAsyncQueue;
+var loadAsyncQueue = process._loadAsyncQueue;
+var unloadAsyncQueue = process._unloadAsyncQueue;
+
+// Do a little housekeeping.
@othiym23
othiym23 Oct 4, 2013

I was under the impression that using delete was a nearly surefire way to mess up the hidden class and cause IC problems.

@othiym23 othiym23 commented on an outdated diff Oct 4, 2013
lib/timers.js
try {
+ if (hasQueue)
+ loadAsyncQueue(first);
if (domain)
@othiym23
othiym23 Oct 4, 2013

Couldn't you replace this with callbacks.before on the domain asyncListener?

@othiym23 othiym23 commented on an outdated diff Oct 4, 2013
lib/timers.js
if (domain)
domain.enter();
- var threw = true;
- first._onTimeout();
+ threw = true;
+ ret = first._onTimeout();
+ if (hasQueue)
+ unloadAsyncQueue(first, ret);
if (domain)
@othiym23
othiym23 Oct 4, 2013

Same as comment on line 104, but for callbacks.exit.

@othiym23 othiym23 commented on an outdated diff Oct 4, 2013
lib/timers.js
immediateQueue = {};
L.init(immediateQueue);
while (L.isEmpty(queue) === false) {
- var immediate = L.shift(queue);
- var domain = immediate.domain;
- if (domain) domain.enter();
- immediate._onImmediate();
- if (domain) domain.exit();
+ immediate = L.shift(queue);
+ hasQueue = !!immediate._asyncQueue;
+ domain = immediate.domain;
+ if (hasQueue)
+ loadAsyncQueue(immediate);
+ if (domain)
@othiym23
othiym23 Oct 4, 2013

Same dealio as line 104.

@othiym23 othiym23 commented on an outdated diff Oct 4, 2013
lib/timers.js
- var immediate = L.shift(queue);
- var domain = immediate.domain;
- if (domain) domain.enter();
- immediate._onImmediate();
- if (domain) domain.exit();
+ immediate = L.shift(queue);
+ hasQueue = !!immediate._asyncQueue;
+ domain = immediate.domain;
+ if (hasQueue)
+ loadAsyncQueue(immediate);
+ if (domain)
+ domain.enter();
+ ret = immediate._onImmediate();
+ if (hasQueue)
+ unloadAsyncQueue(immediate, ret);
+ if (domain)
@othiym23
othiym23 Oct 4, 2013

See comment on line 104.

@othiym23 othiym23 commented on an outdated diff Oct 4, 2013
lib/timers.js
if (!first._onTimeout) continue;
if (domain && domain._disposed) continue;
try {
+ if (hasQueue)
+ loadAsyncQueue(first);
if (domain) domain.enter();
@othiym23
othiym23 Oct 4, 2013

See line 104's comment.

@othiym23 othiym23 commented on an outdated diff Oct 4, 2013
lib/timers.js
threw = false;
+ if (hasQueue)
+ unloadAsyncQueue(first, ret);
if (domain) domain.exit();
@othiym23
othiym23 Oct 4, 2013

Running out of ways to say see the line on comment 104.

@othiym23 othiym23 commented on the diff Oct 4, 2013
lib/domain.js
+ if (domain._disposed)
+ return true;
+
+ er.domain = domain;
+ er.domainThrown = true;
+ // wrap this in a try/catch so we don't get infinite throwing
+ try {
+ // One of three things will happen here.
+ //
+ // 1. There is a handler, caught = true
+ // 2. There is no handler, caught = false
+ // 3. It throws, caught = false
+ //
+ // If caught is false after this, then there's no need to exit()
+ // the domain, because we're going to crash the process anyway.
+ caught = domain.emit('error', er);
@othiym23
othiym23 Oct 4, 2013

So if the domain error handler throws here, the asyncListener will still be active beyond the lifetime of the error callback. Regardless of what the tests are saying, this looks like a semantic error. What am I missing?

@isaacs
isaacs Oct 30, 2013

Doesn't it just not matter because that means that the process is dying anyway?

@trevnorris
trevnorris Oct 31, 2013

No idea. Pulled that straight from the previous code so didn't analysis it
much.

@trevnorris

All the tests are passing. Though this is in some serious need of cleanup. I'll get to that soon so it can be ready for proper review. Also, I realize some of the stuff looks volatile. I'll be adding additional stuff to cover that.

For example, because of how deeply embedded the request object is from HTTP, domain.remove() will not successfully remove the domain from the request object. This can be fixed.

/cc everyone

@bnoordhuis bnoordhuis commented on an outdated diff Oct 17, 2013
doc/api/process.markdown
@@ -648,3 +648,107 @@ a diff reading, useful for benchmarks and measuring intervals:
}, 1000);
[EventEmitter]: events.html#events_class_events_eventemitter
+
+
+## process.createAsyncListener(asyncListener[, callbacksObj[, storageValue]])
+
+* `asyncListener` {Function}
+* `callbacksObj` {Object}
+* `storageValue` {Value}
+
+Returns a constructed `AsyncListener` object. Which can then be passed to
+`process.addAsyncListener()` and `process.removeAsyncListener()`. Each
+function parameter is as follows:
+
+`asyncListener(storageValue)`: A `Function` called as an asynchronous
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

s/as/when/ ?

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Oct 17, 2013
doc/api/process.markdown
+
+
+## process.createAsyncListener(asyncListener[, callbacksObj[, storageValue]])
+
+* `asyncListener` {Function}
+* `callbacksObj` {Object}
+* `storageValue` {Value}
+
+Returns a constructed `AsyncListener` object. Which can then be passed to
+`process.addAsyncListener()` and `process.removeAsyncListener()`. Each
+function parameter is as follows:
+
+`asyncListener(storageValue)`: A `Function` called as an asynchronous
+event is instantiated. If a `Value` is returned then it will be attached
+to the event and overwrite any value that had been passed to
+`createAsyncListener()`'s `storageValue` argument. If an initial
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

Is there a reason for accepting a value from two places?

@trevnorris
trevnorris Oct 17, 2013

Yeah. Because one can overwrite the other. Posted an example usage on another comment.

@bnoordhuis bnoordhuis commented on an outdated diff Oct 17, 2013
doc/api/process.markdown
+to the event and overwrite any value that had been passed to
+`createAsyncListener()`'s `storageValue` argument. If an initial
+`storageValue` was passed when created, then `asyncListener()` will
+receive that as a function argument.
+
+`callbacksObj`: An `Object` which may contain three optional fields:
+
+* `before(context, storageValue)`: A `Function` that is called immediately
+before the asynchronous callback is about to run. It will be passed both
+the `context` (i.e. `this`) of the calling function and the `storageValue`
+either returned from `asyncListener` or passed during construction (if
+either occurred).
+
+* `after(context, storageValue)`: A `Function` called immediately after
+the asynchronous event's callback has run. Note that if the event's
+callback throws during execution this is will not be called if the error
@bnoordhuis bnoordhuis commented on an outdated diff Oct 17, 2013
doc/api/process.markdown
+`callbacksObj`: An `Object` which may contain three optional fields:
+
+* `before(context, storageValue)`: A `Function` that is called immediately
+before the asynchronous callback is about to run. It will be passed both
+the `context` (i.e. `this`) of the calling function and the `storageValue`
+either returned from `asyncListener` or passed during construction (if
+either occurred).
+
+* `after(context, storageValue)`: A `Function` called immediately after
+the asynchronous event's callback has run. Note that if the event's
+callback throws during execution this is will not be called if the error
+is not handled.
+
+* `error(storageValue, error)`: A `Function` called if the event's
+callback threw. If `error` returns `true` then Node will assume the error
+has been properly handled and resume execution normally. In the case
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

s/In the case/When/ ?

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Oct 17, 2013
lib/domain.js
+ process.removeAsyncListener(domain._listener);
+
+ // Exit all domains on the stack. Uncaught exceptions end the
+ // current tick and no domains should be left on the stack
+ // between ticks.
+ stack.length = 0;
+ exports.active = process.domain = null;
+ } catch (er2) {
+ // The domain error handler threw! oh no!
+ // See if another domain can catch THIS error,
+ // or else crash on the original one.
+ // If the user already exited it, then don't double-exit.
+ if (domain === exports.active) {
+ stack.pop();
+ }
+ // TODO(trevnorris): bad logic
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

Bad logic how?

@trevnorris
trevnorris Oct 17, 2013

cannot for the life of me remember why I put that.

@bnoordhuis bnoordhuis commented on an outdated diff Oct 17, 2013
lib/domain.js
this.members.splice(index, 1);
+
+ // First check if the ee is a handle itself.
+ if (ee !== process && ee.removeAsyncListener)
+ ee.removeAsyncListener(this._listener);
+
+ // Manuall remove the asyncListener from the handle, if possible.
@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Oct 17, 2013
lib/domain.js
this.members.splice(index, 1);
+
+ // First check if the ee is a handle itself.
+ if (ee !== process && ee.removeAsyncListener)
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

You could skip the ee !== process check if you renamed the internal removeAsyncListener() function, right?

@trevnorris
trevnorris Oct 17, 2013

Actually the check isn't necessary at all. Was in one of my initial implementations, but it's been worked out since then.

@bnoordhuis bnoordhuis commented on the diff Oct 17, 2013
lib/domain.js
+ domainThrown: false,
+ domain: self
+ });
+ self.emit('error', er);
+ return;
+ }
+
+ var len = fnargs.length;
+ var args = [];
+ var i, ret;
+
+ self.enter();
+ if (fnargs.length > 1) {
+ for (i = 1; i < fnargs.length; i++)
+ args.push(fnargs[i]);
+ ret = cb.apply(_this, args);
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

There's no need for the args array here, right? If I read the code right, fnargs is an arguments object so fn.apply(obj, fnargs) should do the right thing.

@bnoordhuis
bnoordhuis Oct 17, 2013 Member

Wait, it's because you're skipping the first argument. Isn't Array#slice() faster?

@trevnorris
trevnorris Oct 17, 2013

I'm passing arguments directly, which doesn't have splice(). Then I'd have to call Array.prototype.splice.call(args, 1) which isn't faster for the usual case (1-3 args).

@bnoordhuis bnoordhuis commented on the diff Oct 17, 2013
lib/domain.js
- default:
- // slower for less common case: cb(er, foo, bar, baz, ...)
- args = new Array(len - 1);
- for (var i = 1; i < len; i++) {
- args[i - 1] = arguments[i];
- }
- break;
- }
- self.enter();
- var ret = cb.apply(this, args);
- self.exit();
- return ret;
- }
+ self.enter();
+ if (fnargs.length > 0)
+ ret = cb.apply(_this, fnargs);
@trevnorris
trevnorris Oct 17, 2013

What's wrong here?

@bnoordhuis
bnoordhuis Oct 18, 2013 Member

Nothing - it was in reference to that bit of code above where you use an array. Never mind.

@bnoordhuis bnoordhuis and 2 others commented on an outdated diff Oct 17, 2013
lib/events.js
- if (!er) er = new TypeError('Uncaught, unspecified "error" event.');
- er.domainEmitter = this;
- er.domain = this.domain;
- er.domainThrown = false;
- this.domain.emit('error', er);
- } else if (er instanceof Error) {
- throw er; // Unhandled 'error' event
- } else {
- throw TypeError('Uncaught, unspecified "error" event.');
- }
- return false;
+ if (type === 'error' && !this._events.error) {
+ er = arguments[1];
+ if (this.domain) {
+ if (!er)
+ er = new TypeError('Uncaught, unspecified "error" event.');
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

I know you didn't introduce this but... TypeError?

@trevnorris
trevnorris Oct 17, 2013

Whatev's. Doesn't make sense to me either. I'll change it to new Error().

@isaacs
isaacs Oct 28, 2013

It's a TypeError because it means that you did ev.emit('error') with no error object.

@trevnorris
trevnorris Oct 29, 2013

Heh. Ben pointed out the same thing. That code was there before. I though I
had changed it to just Error.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Oct 17, 2013
lib/timers.js
};
+function timerAddAsyncListener(obj) {
+ if (!this._asyncQueue)
+ this._asyncQueue = [];
+ var queue = this._asyncQueue;
+ for (var i = 0; i < queue.length; i++) {
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

O(n) scans are O(n)...

@trevnorris
trevnorris Oct 17, 2013

Yeah. And hopefully n isn't more than 1. Maybe 2. Could have done a if (queue.length === 1) check, but the code clutter for a feature that's already going to kill your performance isn't on my list of necessaries.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Oct 17, 2013
src/async-wrap-inl.h
+inline void AsyncWrap::AddMethods(v8::Handle<v8::FunctionTemplate> t) {
+ NODE_SET_PROTOTYPE_METHOD(t,
+ "addAsyncListener",
+ AddAsyncListener<TYPE>);
+ NODE_SET_PROTOTYPE_METHOD(t,
+ "removeAsyncListener",
+ RemoveAsyncListener<TYPE>);
+}
+
+
+inline uint32_t AsyncWrap::async_flags() const {
+ return async_flags_;
+}
+
+
+inline void AsyncWrap::set_flag(int flag) {
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

It's a good habit to use unsigned ints for values that you're going to bit-manipulate (and in general - if a value is never < 0, there's no reason for it to be signed.)

@trevnorris
trevnorris Oct 17, 2013

Oh, good point. Thanks.

@bnoordhuis bnoordhuis and 2 others commented on an outdated diff Oct 17, 2013
lib/timers.js
@@ -261,10 +307,12 @@ exports.setInterval = function(callback, repeat) {
exports.clearInterval = function(timer) {
- if (timer && timer._repeat) {
- timer._repeat = false;
- clearTimeout(timer);
- }
+ process.nextTick(function() {
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

That rather breaks the clearInterval() contract, doesn't it?

@trevnorris
trevnorris Oct 17, 2013

It's still guaranteed to be cleared before it's possible for the setInterval() to run again.

@isaacs
isaacs Oct 28, 2013

Not necessarily. Qv: https://gist.github.com/isaacs/7206752

Why is this nextTick necessary? It's a bug waiting to happen.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Oct 17, 2013
src/node.cc
- if (env->using_domains())
- return;
- env->set_using_domains(true);
+ assert(args[0]->IsObject() &&
+ args[1]->IsFunction() &&
+ args[2]->IsFunction() &&
+ args[3]->IsFunction() &&
+ args[4]->IsFunction() &&
+ args[5]->IsFunction());
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

It's arguably better to break this out into one assert statement per check. The way it's written now, if the assert hits, you know one of the six arguments is not what it's supposed to be but you don't know which one.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Oct 17, 2013
src/node.js
+ process.addAsyncListener = addAsyncListener;
+ process.removeAsyncListener = removeAsyncListener;
+
+ // Setup shared objects/callbacks with native layer.
+ process._setupAsyncListener(asyncFlags,
+ runAsyncQueue,
+ loadAsyncQueue,
+ unloadAsyncQueue,
+ pushListener,
+ stripListener);
+
+ // Run all the async listeners attached when an asynchronous event is
+ // instantiated.
+ function runAsyncQueue(context) {
+ var queue = [];
+ var queue_item, item, i, value;
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

Snake case upsets the JS style police.

@trevnorris
trevnorris Oct 17, 2013

hah, yeah. sometimes have a hard time context switching between c++ and js variable styles. :P

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Oct 17, 2013
src/node.js
+ loadAsyncQueue,
+ unloadAsyncQueue,
+ pushListener,
+ stripListener);
+
+ // Run all the async listeners attached when an asynchronous event is
+ // instantiated.
+ function runAsyncQueue(context) {
+ var queue = [];
+ var queue_item, item, i, value;
+
+ inAsyncTick = true;
+ for (i = 0; i < asyncQueue.length; i++) {
+ queue_item = asyncQueue[i];
+ // TODO(trevnorris): I should only have to create this object if
+ // the value field changes.
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

You should address this before landing the patch; the current approach can, in theory, create a lot of garbage.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Oct 17, 2013
src/node.js
+ // the value field changes.
+ item = {
+ callbacks: queue_item.callbacks,
+ value: queue_item.value,
+ listener: queue_item.listener,
+ uid: queue_item.uid
+ };
+ // Not passing "this" context because it hasn't actually been
+ // instantiated yet, so accessing some of the object properties
+ // can cause a segfault.
+ // Passing the original value will allow users to manipulate the
+ // original value object, while also allowing them to return a
+ // new value for current async call tracking.
+ value = item.listener(queue_item.value);
+ if (typeof value !== 'undefined')
+ item.value = value;
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

This seems kind of pointless, frankly. Let the user use an object if he wants mutable data.

@trevnorris
trevnorris Oct 17, 2013

I'm curious how you mean? The point of being able to initially set a storageValue then overwrite it is for (as a contrived example) you can do things like the following:

var storage = { cntr: 0 };
process.addAsyncListener(function(storage) {
  // keep track of some global states, w/o relying on being able to access
  // the storage in a bubbled scope.
  storage.cntr++;
  // Now return a new object that can help keep state for callbacks.
  return {};
}, { /* callbacks */ }, storage);

Also, I'm not restricting the value to just an Object. They can return pretty much anything.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Oct 17, 2013
src/node.js
+ queue_item = asyncQueue[i];
+ // TODO(trevnorris): I should only have to create this object if
+ // the value field changes.
+ item = {
+ callbacks: queue_item.callbacks,
+ value: queue_item.value,
+ listener: queue_item.listener,
+ uid: queue_item.uid
+ };
+ // Not passing "this" context because it hasn't actually been
+ // instantiated yet, so accessing some of the object properties
+ // can cause a segfault.
+ // Passing the original value will allow users to manipulate the
+ // original value object, while also allowing them to return a
+ // new value for current async call tracking.
+ value = item.listener(queue_item.value);
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

What happens when the listener modifies the queue?

@trevnorris
trevnorris Oct 17, 2013

Well, we're passing the storageValue, but asking what happens if they screw with ._asyncQueue is the same as asking what if a user does buffer.parent = undefined. They just shouldn't do it.

@bnoordhuis bnoordhuis commented on an outdated diff Oct 17, 2013
src/node.js
+ uid: uid++
+ };
+ }
+
+ // Add a listener to the current stack.
+ function addAsyncListener(listener, callbacks, value) {
+ // Accept new listeners or previous created listeners.
+ if (typeof listener === 'function')
+ callbacks = createAsyncListener(listener, callbacks, value);
+ else
+ callbacks = listener;
+
+ // Make sure the callback doesn't already exist in the queue.
+ var inQueue = false;
+ for (var i = 0; i < asyncQueue.length; i++) {
+ if (callbacks.uid === asyncQueue[i].uid) {
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

Another O(n) scan.

@bnoordhuis bnoordhuis commented on the diff Oct 17, 2013
src/node_crypto.cc
@@ -830,7 +830,7 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo<Value>& args) {
HandleScope scope(node_isolate);
Base* w = static_cast<Base*>(SSL_get_app_data(s));
- Environment* env = w->env();
+ Environment* env = w->ssl_env();
@bnoordhuis
bnoordhuis Oct 17, 2013 Member

Are you storing a pointer to the env twice now?

@bnoordhuis
bnoordhuis Oct 17, 2013 Member

Ah no, I see it now - the joys of multiple inheritance.

@trevnorris
trevnorris Oct 17, 2013

Yeah. I feel like this is a crap way of handling the problem, but couldn't figure out a better one.

@trevnorris trevnorris referenced this pull request Oct 28, 2013
Closed

domains are verbose #4230

@isaacs isaacs and 1 other commented on an outdated diff Oct 29, 2013
src/node.js
tickInfo[kIndex] = 0;
}
- // run callbacks that have no domain
- // using domains will cause this to be overridden
+ // Run callbacks that have no domain.
+ // Using domains will cause this to be overridden.
@isaacs
isaacs Oct 29, 2013

Is this comment still true?

@isaacs
isaacs commented Oct 29, 2013

I'm fine with this, and I think it's ready to go, pending the following:

  1. @tjfontaine is doing a master v0.11 build today, let's land this after that goes out.
  2. @bnoordhuis signs off on C++ side. (Seems close, yes?)
  3. @piscisaureus puts his complaints/suggestions here (or LGTM's it).
@piscisaureus
Member

I'm fine with it too barring the following:

  • Spurious listener callbacks are made for objects that don't actually do anything asynchronous (e.g. Hmac and ContextifyScript).
  • It is not possible to tell when a "context" that was passed to the listener ends. My proposal would be:
    • pass an isFinal flag to the before and after callbacks. That is, when a ReqWrap's complete callback is made, or a HandleWrap's close callback.
    • Ensure that the onclose callback is called for all HandleWraps. Currently there are some that don't always (Timer and Timeout objects) or that rely on the weak callback (StatWatcher).
@indutny
Member
indutny commented Oct 29, 2013

I'd like to have a deeper look at it as well, if you don't mind, @isaacs .

@trevnorris

@piscisaureus I have a WIP right now for those. just cherry-picking a few other commits into another PR first to shrink this. then i'll post the work for you to review.

@isaacs
isaacs commented Oct 29, 2013

@piscisaureus Are you ok with those things being fixed in subsequent patches, provided that @trevnorris commits to doing them and I commit to not releasing 0.12 without them?

IMO, isFinal is a nice to have (but can clearly be a subsequent pre-v0.12 addition), and the spurious emissions are bugs, but relatively low-harm.

@trevnorris

@isaacs I do have a fix for the spurious emissions. was just cleaning up some other code first. i'll land that commit shortly.

@trevnorris

@isaacs actually, nevermind the fix for now. i'm actively working on both issues that @piscisaureus has, but don't want to rush them. I created #6437 to land the first 5 commits of this PR to help it from being so large, but guess if this is already completely reviewed then no reason to worry about that.

Also, thanks everyone for reviewing this. I'll really try to keep my 4 digit changed lines PR's down to a minimum in the future. :)

@trevnorris

Ok everyone. Commits have been cleaned up and added some additional documentation. Unless someone else sees a bug, I'm ready to have this merged.

@isaacs
isaacs commented Oct 31, 2013

@trevnorris Last blocker, this fails a lot:

i=0; while ./node test/simple/test-asynclistener-error.js && [ $i -lt 100 ]; do echo $i; let i++; done

It looks like there's a race condition, where a throw in one setTimeout section will prevent other setTimeout callbacks attached to the same Timer object (ie, with the same timeout msecs value).

Running each of the sections of the test individually works fine, but running them all together fails within about 5-10 runs on average.

@trevnorris

Interesting. Thanks, I'll have a look at what's going on.

trevnorris and others added some commits Sep 24, 2013
@trevnorris trevnorris node: add AsyncListener support
AsyncListener is a JS API that works in tandem with the AsyncWrap class
to allow the user to be alerted to key events in the life cycle of an
asynchronous event. The AsyncWrap class has its own MakeCallback
implementation that core will be migrated to use, and uses state sharing
techniques to allow quicker communication between JS and C++ whether the
async event callbacks need to be called.
efa62fd
@trevnorris trevnorris async-wrap: integrate with WeakObject
Making WeakObject inherit from AsyncWrap allows us to peak into almost
all the MakeCallback calls in Node internals.
8b8e3b6
@trevnorris trevnorris async-wrap: add methods to udp/tcp/pipe/timers
Now it's possible to add/remove an async listener to an individual
handle created by UDP, TCP, Pipe or Timer.
3c77151
@trevnorris trevnorris cares: add AsyncListener support 56c558c
@trevnorris trevnorris crypto: convert RandomBytesRequest to a class
Since RandomBytesRequest makes a call to MakeCallback, needed it to be
a class so AsyncWrap could handle any async listeners.

Also added a simple test for an issue had during implementation where
the memory was being released and returned.
83e3635
@trevnorris trevnorris crypto: convert pbkdf2_req to a class
pbkdf2_req has been renamed to PBKDF2Request and converted to a class.
It now uses AsyncWrap::MakeCallback.

Also includes, using env()->ondone_string() instead of "ondone" and
using malloc instead of new char[].
79ca3cf
@trevnorris trevnorris domain: use AsyncListener API
The domain module has been switched over to use the domain module API as
much as currently possible. There are still some hooks in the
EventEmitter, but hopefully we can remove those in the future.
bfeb189
@groundwater @trevnorris groundwater test: add additional async listener tests 584179f
@trevnorris trevnorris doc: add docs for AsyncListeners
Documentation has been added on how to use the AsyncListener API.
0a0577e
@isaacs
isaacs commented Nov 1, 2013

Everything relevant is landed in master now, right? Reopen if there's still work to do here.

Thanks for all the work on this, @othiym23, @jacobgroundwater, and especially @trevnorris.

@isaacs isaacs closed this Nov 1, 2013
@trevnorris trevnorris deleted the unknown repository branch Nov 1, 2013
@refack
Member
refack commented Jun 25, 2014

@trevnorris why arenet incomming events created out-of band?
for the following code the second assert fails

                namespace.set('test', TEST_VALUE);
                server = net.createServer();
                server.on('connection', function OnServerConnection(socket) {
                    expect(namespace.get('test')).equal(TEST_VALUE, "state should be preserved");  // Still have state
                    socket.on("data", function OnServerSocketData(data) {
                        expect(namespace.get('test')).equal(TEST_VALUE, "state should still be preserved :(");  // cls is lost
@bnoordhuis
Member

@refack I'm not sure why you direct that question to me.

@refack
Member
refack commented Jun 25, 2014

Sorry ment to ping @trevnorris

@trevnorris

@refack If I understand the question, it's because internally we don't know whether you're creating a Pipe or a TCP connection until you call listen(). It's a total PITA. I have another PR open that does some work on this, but still under debate how it should be merged.

@RobinQu
RobinQu commented Jun 1, 2015

Is it landed on any released version?
I still cannot find it in the latest(v0.12.2)

@trevnorris

@RobinQu Never landed.

@gobwas
gobwas commented Jun 30, 2015

So, will node have this feature some day?

@trevnorris

@gobwas It partially exists as process.binding('async_wrap'). Though the ability to capture exceptions was removed. Became a massive hair ball. Would still like to do this, but time constraints.

EDIT: It's in io.js, not node.js.

@gobwas
gobwas commented Jun 30, 2015

Thank you, @trevnorris!

Does this feature present in iojs docs?

Any way, I am asking about async-listener functionality at the moment..?

@trevnorris

Oh, that died a horrible death. After 3 months of work I still couldn't figure out a few key edge cases. So instead I've been working on the set of hooks necessary to implement a feature like async-listener. This way others can experiment and publish their own modules.

@gergelyke

Hi @trevnorris ,

could you give us an update on the current status of this? What would be your suggestion to use these days if we want to active the same functionality?

Thanks!

cc @peteyy

@trevnorris

@gergelyke Unfortunately there isn't an alternative. Will have to continue using domains for the time being. Though discussion about reimplementing this has started back up. Hopefully it will lead to something.

@gergelyke

thanks!

@trevnorris trevnorris referenced this pull request in nodejs/LTS Dec 3, 2015
Closed

AsyncWrap for LTS Argon #59

@naorye
naorye commented Dec 10, 2015

What is wrong with the Domain solution?

@trevnorris

@naorye It's a complete hack that injects itself all over core code. By design it can't properly catch everything. There are edge cases it's missing. This is because it's injected at the JS API level, not at the native level where the asynchronous requests are actually made.

@Olegas
Olegas commented Dec 12, 2015

What alternatives are now being discussed?

@trevnorris trevnorris changed the title from implement domain-like hooks for userland to implement domain-like hooks (asynclistener) for userland Jul 25, 2016
@DeTeam DeTeam referenced this pull request in othiym23/node-continuation-local-storage Dec 19, 2016
Open

is this part of nodejs or we still need to install this middleware from npm? #88

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment