Skip to content
This repository has been archived by the owner. It is now read-only.

Allow passing specific PROVIDERs to AsyncListener callbacks #7145

Closed
wants to merge 5 commits into from

Conversation

@trevnorris
Copy link

trevnorris commented Feb 18, 2014

This allows user to pass specific "providers" to {add,create}AsyncListener() which will allow them to track only specific operations.

While the functionality is working now, I'm sure a lot of fine tuning will be done as users get their hands on it.

@trevnorris trevnorris added this to the v0.12 milestone Feb 18, 2014
@trevnorris trevnorris self-assigned this Feb 18, 2014
The `NEXTTICK` `ASYNC_PROVIDER` will always fire regardless, because of the
implicit usage of [`process.nextTick()`][] within core code. This makes it
difficult to determine the true origin of the call. We hope to correct this
in the future.

This comment has been minimized.

Copy link
@tjfontaine

tjfontaine Feb 25, 2014

Including these last two lines is not necessary, just state the fact and leave it at that.

callbacks will cease to fire on the events they've been registered.
Subsequently, any asynchronous events fired during the execution of a
callback will also have the same `AsyncListener` callbacks attached for
future execution. For example:

This comment has been minimized.

Copy link
@tjfontaine

tjfontaine Feb 25, 2014

How about:

Callbacks are associated with handles upon their creation, deregistering callbacks does not remove them from handles in flight, only they won't be added to any newly created handles. Therefore after deregistration your callbacks may fire.

This comment has been minimized.

Copy link
@trevnorris

trevnorris Mar 7, 2014

Author

I think by:

[...] deregistering callbacks [...]

You meant:

[...] deregistering an AsyncListener [...]

Since "callbacks" feels like the callbacks passed to the asynchronous events that will fire in the future.

Feel slight confusion on the line:

[...] does not remove them from handles in flight, only they won't be added to any newly created handles.

Because I feel like that implies that the AsyncListener callbacks will not fire on any further creations of the asynchronous call stack on the handles where it was initially registered. When in fact the callbacks will continue to fire for all asynchronous "events" that originate from the handle which the AsyncListener had been added, before having been removed from the activeContext and the contextStack.

Maybe something like the following:

An active AsyncListener (i.e. passed to tracing.addAsyncListener()) is attached to an asynchronous handle, when the handle is created. Removing the AsyncListener simply prevents it from being added to any additional handles within the asynchronous scope where it was removed. Though this does not prevent the AsyncListener from propagating along the asynchronous call stack along the handle on which it was attached. For example:

var tracing = require('tracing');
var addAL = tracing.addAsyncListener;
var removeAL = tracing.removeAsyncListener;

// The AsyncListener will now begin to attach to any
var listener = addAL(callbacksObj);
setTimeout(function() {
  process.nextTick(function() {
    throw new Error('this will be caught');
  });
  setImmediate(function() {
    removeAL(listener);
    process.nextTick(function() {
      throw new Error('this will not be caught');
    });
    throw new Error('this will not be caught');
  });
  throw new Error('this will be caught');
});
TTY: 1 << 13,
UDP: 1 << 14,
ZLIB: 1 << 15
};

This comment has been minimized.

Copy link
@tjfontaine

tjfontaine Feb 25, 2014

I'd rather not have to maintain this list in two places, what about doing something like tjfontaine@d4b33de#diff-df673cc6ba0e8183c4b07ab96df8c4a9R298 instead?

This comment has been minimized.

Copy link
@trevnorris

trevnorris Mar 7, 2014

Author

Noted. I'll take care of something like this before it is merged.

This comment has been minimized.

Copy link
@refack

refack Aug 2, 2014

Member

@trevnorris Can I take a swing at this one?
I'd move them both to node-contants.cc (seems reasable someone might want to reuse it), and wire a NODE_DEFINE_CONSTANTS_MAP macro.

@groundwater

This comment has been minimized.

Copy link

groundwater commented Mar 12, 2014

I have come across an inconsistency between the async polyfill and the native async-listener interface. This currently breaks some of the CLS tests.

It appears that some create callbacks occur outside the scope of a before/after context. For example, here is what continuation-local-storage is expecting from async-listener:

(scenario: three asynchronous calls in a single continuation chain)

  create[1]
      |
      |
  before[1]
      |      create[2]
   after[1]      |
                 |
             before[2]
                 |      create[3]
              after[2]      |
                            |
                        before[3]
                            |
                         after[3]

In order for the contexts to be set up correctly to pass state through the continuation chain, each subsequent continuation has to be able to get at the context set by the previous ones. Therefore, each create callback has to be run after the previous before callback, and before the previous after callback.

The native async-listener breaks this contract, as illustrated in a trace below:

--> MAIN
  * SET STATE
  + CREATE 1
  + CREATE 2
<-- MAIN

--> BEFORE 2
  + CREATE 3
  + CREATE 4
<-- AFTER  2

  + CREATE 5

--> BEFORE 1
  * ASSERT STATE PRESERVED
  + CREATE 6
<-- AFTER  1

The above trace was generated while debugging the CLS problem.

A more detailed trace, along with a code example is available here https://gist.github.com/groundwater/e791f503e21e7c44f2f1

The way the current pollyfill works, the above CREATE 5 event would occur between BEFORE/AFTER 1. This lets CLS link any events in the onConnect handler with events in the onData handler.

@othiym23 and I have tried several variations on solving this, but there seems to always be a corner case that can break it. I am skeptical about moving CLS to support this API, as there seem to be a lot of corner cases to handle.

Given that we are predicating a lot of work on the ability of our (or any) tracing module to consume ALs, I just wanted to make sure it's clear that this solution is going to be difficult for us to consume.

Is the create statement supposed to happen outside of a before/after, or is that a bug which we need new tests for? I don't want to hold up the 0.12 release, but this is a critical bug.

cc/ @tjfontaine @trevnorris

@tjfontaine

This comment has been minimized.

Copy link

tjfontaine commented Mar 13, 2014

Ok this is a serious issue --

Coming in from the libuv event loop instantiating a handle_wrap results in a create callback that can't be wrapped in a before/after context load because those only happen for transitions from MakeCallback.

This basically makes use case for CLS impossible.

I think we will have to make this API private.

@trevnorris

This comment has been minimized.

Copy link
Author

trevnorris commented Mar 13, 2014

I'm aware of the following issue:

var l = tracing.addAsyncListener(callbacks);
net.createServer(function() {
  // This connection and any incoming data requests won't
  // be linked to the AL that wrapped the server.
}).listen(8000);
tracing.removeAsyncListener(l);

It's something I've been working on solving.

@trevnorris

This comment has been minimized.

Copy link
Author

trevnorris commented Mar 13, 2014

@tjfontaine please refrain from making sweeping statements like "Ok this is a serious issue" without speaking with the developer that's actually spent his life in this patch.

@trevnorris

This comment has been minimized.

Copy link
Author

trevnorris commented Mar 21, 2014

The issue brought up by @groundwater has been resolved. I need to double check and see if there are any other cases where this will occur in source, and also do some cleanup of how the operation is performed, but it's working.

The last "bug" for me is an edge case when a listener is removed in the middle of an asynchronous call stack.

@@ -41,30 +41,63 @@ inline AsyncWrap::AsyncWrap(Environment* env,
: BaseObject(env, object),
async_flags_(NO_OPTIONS),
provider_type_(provider) {
if (!env->has_async_listener())
AsyncWrap* parent = env->async_wrap_parent_class();

This comment has been minimized.

Copy link
@tonistiigi

tonistiigi Apr 6, 2014

There is nothing initializing async_wrap_parent_ to NULL or reset_async_wrap_parent_class() is not called every time. details

This comment has been minimized.

Copy link
@trevnorris

trevnorris Apr 23, 2014

Author

What system are you running on? I can't reproduce the issue you're having.

This comment has been minimized.

Copy link
@trevnorris

trevnorris Apr 23, 2014

Author

@tonistiigi can you run your tests again? it should be fixed by 3f812ac.

This comment has been minimized.

Copy link
@tonistiigi

tonistiigi Apr 24, 2014

Yes, seems to work fine now. This was on OSX.

This comment has been minimized.

Copy link
@trevnorris

trevnorris Apr 24, 2014

Author

great. thanks for reporting this. :)

trevnorris added 5 commits Mar 26, 2014
* Generalize the ASYNC_PROVIDER being passed by then category to which
  it belongs. Instead of having specialized providers for classes like
  ConnectWrap.
* When some classes are instantiated from a libuv callback there is no
  activeContext, so manually set the parent class that contains the
  context that should be used for class instantiation. This will allow
  proper propagation.
* Force before/after callbacks to run when a class is instantiated, not
  only when a callback is about to be called from MakeCallback.
* Add _removeAsyncQueue() method to handles so when there are no longer
  any items in the queue async_flags_ can be reset.
* Manually track additional states in Environment. For example: the
  length of the activeContext queue, the provider type of the
  activeContext or whether any AsyncListener callbacks are being
  processed.
* Pass if the error was handled as the fourth argument to the error
  callback handler.
* Move all properties to single _async object.
* Only run callbacks for watched providers.
* Each asynchronous call chain is independent and the listener can be
  removed at any time for that specific branch in the call stack.
* Each AsyncListener tracks its own watched providers. These are
  accumulated on _async.watchedProviders for quick checks when the
  run/load/unload functions are called.
* When an AsyncListener is removed, it's removed from the activeContext
  and from every context in the contextStack.
If an AsyncListener was added in the middle of an async execution chain
then it was possible for the after callback to run w/o the before
callback having had a chance to run.

This make sure the before callback has a chance to run, or the after
callback will be skipped.
This is to prevent an edge case that causees EXC_BAD_ACCESS.
@trevnorris

This comment has been minimized.

Copy link
Author

trevnorris commented Aug 8, 2014

Closing this. I'm going to keep the branch open on my repo as to not loose a lot of the JS logic that I spent so much time on. But for now it'll be removed, and I'll open another PR for that.

@trevnorris trevnorris closed this Aug 8, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.