Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks initial implementation #11883

Closed
wants to merge 12 commits into from

Conversation

trevnorris
Copy link
Contributor

@trevnorris trevnorris commented Mar 16, 2017

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_wrap, async_hooks and a little bit of everything else

This PR supersedes my other PR for async_hooks support. The implementation has been cleaned up and @thlorenz has helped create much better tests (though they may need to be changed from their current location) and which accounts for ~3300 lines of the change. Documentation does exist, but needs to be cleaned up some before it's included. The code is ready for initial review.

For anyone reviewing, I carefully went through and divided all the changes into logical commits that should all pass. So while reviewing feel free to use each commit individually. Though GitHub fails at displaying them in the correct order.

The one aspect of this PR that I don't like but have gone through hell trying to get around to no avail is the usage of initTriggerId(). It's basically a way to propagate the "triggerId" to a constructor. The code comments explain some of those difficulties, but I hope someone can come up with something better.

@nodejs/ctc

Refs: #8531

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

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. async_wrap build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 16, 2017
@AndreasMadsen
Copy link
Member

AndreasMadsen commented Mar 16, 2017

Would it make sense to move 556cc9b6d047dd6d3ba95b4390222d11018a52b5 and 922bcec92c72804f24b609133db984a1959eb7ed to a separate PR? These commits appear to be completely separate to async_hooks and since the review process for this PR will be long, it would be nice to get these fixes merged sooner than later.

@addaleax
Copy link
Member

addaleax commented Mar 20, 2017

Agreed, that makes sense. 556cc9b6d047dd6d3ba95b4390222d11018a52b5 is actually exactly the alternative to #11776 that I had in mind (#11776 (comment)).

edit: to be clear, I would still prefer the way 556cc9b6d047dd6d3ba95b4390222d11018a52b5 does it

src/node_zlib.cc Outdated
@@ -73,7 +73,7 @@ void InitZlib(v8::Local<v8::Object> target);
class ZCtx : public AsyncWrap {
public:
ZCtx(Environment* env, Local<Object> wrap, node_zlib_mode mode)
: AsyncWrap(env, wrap, AsyncWrap::PROVIDER_ZLIB),
: AsyncWrap(env, wrap, AsyncWrap::PROVIDER_ZCTX),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to keep this equal to the class names, sure, but I would consider ZLIB the more obvious/readable choice (so renaming the class might actually be better?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can live with that. Will change.

src/udp_wrap.cc Outdated
@@ -65,7 +65,7 @@ class SendWrap : public ReqWrap<uv_udp_send_t> {
SendWrap::SendWrap(Environment* env,
Local<Object> req_wrap_obj,
bool have_callback)
: ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_UDPSENDWRAP),
: ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_SENDWRAP),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ditto, UDPSENDWRAP > SENDWRAP imho)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what I was thinking here. will change.

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same timers comments as the previous PR?

Maybe check the old PR and check the code locations for timers.js there?

!timer._destroyed) {
emitDestroy(timer[async_id_symbol]);
timer._destroyed = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is in the wrong spot again?

Shouldn't it be in unenroll and Timeout#close()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is intentional. The previous implementation was fragile, so I decided it would be better to harden by restricting the scope to only timers created with setImmediate/setTimeout/setInterval. I'd prefer to add a test that shows how not having these calls in unenroll and Timeout#close causes the async graph to be incorrect before attempting to change their locations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this miss net timeouts then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic would lead me to believe so, but I don't have a test to verify that yet

Copy link
Contributor

@Fishrock123 Fishrock123 Apr 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok well, the tests seem to ignore timeouts in the net tests so it was never caught. Run any net server connection and you will quickly find that destroy is never reported on timers for any connection that does not timeout.

Correction, you will see TIMERWRAP but not individual timers.

this[async_id_symbol] = ++async_uid_fields[kAsyncUidCntr];
this[trigger_id_symbol] = initTriggerId();
if (async_hook_fields[kInit] > 0)
emitInit(this[async_id_symbol], 'Timeout', this[trigger_id_symbol], this);
Copy link
Contributor

@Fishrock123 Fishrock123 Mar 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still needs to go into insert(), I'm quite certain.

@trevnorris
Copy link
Contributor Author

@addaleax I've split out that one commit: #11947

@rvagg
Copy link
Member

rvagg commented Mar 22, 2017

@trevnorris CI failures on all Windows versions for parallel/test-async-wrap-getasyncid, e.g. on this one

not ok 2 parallel/test-async-wrap-getasyncid
  ---
  duration_ms: 0.186
  severity: fail
  stack: |-
    (node:6548) [DEP0064] DeprecationWarning: tls.createSecurePair() is deprecated. Please use tls.Socket instead.
    c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015\label\win2016\test\parallel\test-async-wrap-getasyncid.js:176
          throw new Error(`write failed: ${process.binding('uv').errname(err)}`);
          ^
    
    Error: write failed: EPIPE
        at TCPConnectWrap.req.oncomplete.common.mustCall (c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015\label\win2016\test\parallel\test-async-wrap-getasyncid.js:176:13)
        at TCPConnectWrap.oncomplete (c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015\label\win2016\test\common.js:517:15)

Also, I can't run another CI because this needs rebasing already https://ci.nodejs.org/job/node-test-commit/8612/console

@Fishrock123
Copy link
Contributor

@@ -93,6 +94,7 @@ function Agent(options) {
self.freeSockets[name] = freeSockets;
socket.setKeepAlive(true, self.keepAliveMsecs);
socket.unref();
socket._asyncId = -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the _asyncId properties use a Symbol instead, if only because that increases readability?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing.

@@ -176,6 +180,7 @@ Agent.prototype.addRequest = function addRequest(req, options) {
// If we are under maxSockets create a new one.
this.createSocket(req, options, function(err, newSocket) {
if (err) {
setInitTriggerId(newSocket._handle.getAsyncId());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure I understand this, this call is here to tell the subsequent process.nextTick who is responsible for the call?

Have you tried making an internal version of process.nextTick() that takes an async ID? That seems like a more direct approach…

Even it that’s not feasible (for now), a shorthand for setInitTriggerId() + process.nextTick() would be helpful for readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generalized the call b/c of the not-so-obvious way that constructors like TCPConnectWrap() works. setInitTriggerId() is a little devil child that I'd love to banish to the depths of hell, but every time I've attempted to remove it there are complications.

Anyhow, I had considered creating an internal nextTick() that accepts the triggerId but didn't know how people would feel about it. I'll be happy to add it in if others are okay with it being introduced.

// or disables a hook while hooks are being processed.
var tmp_active_hooks_array = null;
// Keep track of the field counds held in tmp_active_hooks_array.
var tmp_async_hook_fields = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in the comment: counts


// Used to fatally abort the process if a callback throws.
function fatalError(e) {
if (e instanceof Error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just if (e.stack)? If you want to filter for native errors only, you could also make process.binding('util').isNativeError available

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yup, good call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about if (typeof e.stack === 'string')?

// Public API //

class AsyncHook {
constructor(fns) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably just use constructor({ init, before, after, destroy }) if you want

lib/net.js Outdated
@@ -1278,6 +1308,8 @@ Server.prototype._listen2 = function(address, port, addressType, backlog, fd) {
this._handle = rval;
}

this._asyncId = getNewAsyncId(this._handle);
//this._asyncId = this._handle.getAsyncId();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment supposed to stay?

FIXED_ONE_BYTE_STRING(env->isolate(), #name)).ToLocalChecked(); \
CHECK(name##_v->IsFunction()); \
env->set_async_hooks_##name##_function(name##_v.As<Function>());
GET_HOOK_FN(init);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can you leave a space above this so that it’s more obvious where the macro ends?

argv[2] = Number::New(env->isolate(), parent->get_uid());
argv[3] = parent->object();
}
env()->destroy_ids_list()->push_back(get_id());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shares quite a bit of code with AddIdToDestroyList, any chance those could be joined into a common helper?

src/env-inl.h Outdated
uid_fields_() {
v8::HandleScope handle_scope(isolate_);

// kAsyncUidCntr should start at 1 because that'll be the id for bootstrap.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“bootstrap” as in “bootstrap_node.js”?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all sync work at startup will have that id, afaik

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bootstrap meaning all the code that runs before the first uv_run() is executed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should specify that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

V(UDPSENDWRAP) \
V(UDPWRAP) \
V(WRITEWRAP) \
V(ZLIB)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do callbacks from native addons fit into this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AsyncWrap uses an enum so it can store and more easily access the char* of each provider's name for provider_type(), and a couple other things. There will be a native API for async hooks, but I'm not including that in this PR. After this lands I'll post the native API PR.

@AndreasMadsen
Copy link
Member

V8 5.7 (#11752) has landed with PromiseHooks. See design document: https://docs.google.com/document/d/1rda3yKGHimKIhg5YeoAmCOtyURgsbTH_qaYR79FELlk/edit?usp=sharing

addaleax pushed a commit that referenced this pull request May 10, 2017
The number of ids is limited to 2^53-1 regardless of whether an int64_t
is used or not because JS is limited to a double. So to make conversion
simpler, track ids internally as a double. This will also make life
simpler when this is eventually exposed to JS via a Float64Array.

Rename AsyncWrap::get_uid() to AsyncWrap::get_id().

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
RandomBytes and PBKDF2 were using the same "generic" ObjectTemplate for
construction. Instead create one for each that is properly named.

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
Instead of wrapping several providers into PROVIDER_CRYPTO, have them
all be named after their class. Rename other providers to also match
their class names. With the exception of Parser. Which is actually
HTTPParser.

Add PROVIDER_LENGTH to make better checks in WrapperInfo().

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
Allow handles to retrieve their own uid's by adding a new method on the
FunctionTemplates. Implementation of these into all other classes will
come in a future commit.

Add the method AsyncWrap::GetAsyncId() to all inheriting class objects
so the uid of the handle can be retrieved from JS.

In all applicable locations, run ClearWrap() on the object holding the
pointer so that it never points to invalid memory and make sure Wrap()
is always run so the class pointer is correctly attached to the object
and can be retrieved so GetAsyncId() can be run.

In many places a class instance was not removing its own pointer from
object() in the destructor. This left an invalid pointer in the JS
object that could cause the application to segfault under certain
conditions.

Remove ClearWrap() from ReqWrap for continuity. The ReqWrap constructor
was not the one to call Wrap(), so it shouldn't be the one to call
ClearWrap().

Wrap() has been added to all constructors that inherit from AsyncWrap.
Normally it's the child most class. Except in the case of HandleWrap.
Which must be the constructor that runs Wrap() because the class pointer
is retrieved for certain calls and because other child classes have
multiple inheritance to pointer to the HandleWrap needs to be stored.

ClearWrap() has been placed in all FunctionTemplate constructors so that
no random values are returned when running getAsyncId(). ClearWrap() has
also been placed in all class destructors, except in those that use
MakeWeak() because the destructor will run during GC. Making the
object() inaccessible.

It could be simplified to where AsyncWrap sets the internal pointer,
then if an inheriting class needs one of it's own it could set it again.
But the inverse would need to be true also, where AsyncWrap then also
runs ClearWeak. Unforunately because some of the handles are cleaned up
during GC that's impossible. Also in the case of ReqWrap it runs Reset()
in the destructor, making the object() inaccessible. Meaning,
ClearWrap() must be run by the class that runs Wrap(). There's currently
no generalized way of taking care of this across all instances of
AsyncWrap.

I'd prefer that there be checks in there for these things, but haven't
found a way to place them that wouldn't be just as unreliable.

Add test that checks all resources that can run getAsyncId(). Would like
a way to enforce that any new classes that can also run getAsyncId() are
tested, but don't have one.

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
Changes in the native code for the upcoming async_hooks module. These
have been separated to help with review and testing.

Changes include:

* Introduce an async id stack that tracks recursive calls into async
  execution contexts. For performance reasons the id stack is held as a
  double* and assigned to a Float64Array. If the stack grows too large
  it is then placed in it's own stack and replaced with a new double*.
  This should accommodate arbitrarily large stacks.

  I'm not especially happy with the complexity involved with this async
  id stack, but it's also the fastest and most full proof way of
  handling it that I have found.

* Add helper functions in Environment and AsyncWrap to work with the
  async id stack.

* Add AsyncWrap::Reset() to allow AsyncWrap instances that have been
  placed in a resource pool, instead of being released, to be
  reinitialized. AsyncWrap::AsyncWrap() also now uses Reset() for
  initialization.

* AsyncWrap* parent no longer needs to be passed via the constructor.

* Introduce Environment::AsyncHooks class to contain the needed native
  functionality. This includes the pointer to the async id stack, and
  array of v8::Eternal<v8::String>'s that hold the names of all
  providers, mechanisms for storing/retrieving the trigger id, etc.

* Introduce Environment::AsyncHooks::ExecScope as a way to track the
  current id and trigger id of function execution via RAII.

* If the user passes --abort-on-uncaught-exception then instead of
  throwing the application will print a stack trace and abort.

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
Fill this commit messsage with more details about the change once all
changes are rebased.

* Add lib/async_hooks.js

* Add JS methods to AsyncWrap for handling the async id stack

* Introduce AsyncReset() so that JS functions can reset the id and again
  trigger the init hooks, allow AsyncWrap::Reset() to be called from JS
  via asyncReset().

* Add env variable to test additional things in test/common.js

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
Implement async_hooks support in the following:

* fatalException handler
* process.nextTick
* Timers
* net/dgram/http

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
Async wrap providers tested:

- crypto.randomBytes
- crypto.pbkdf2
- fs event wrap
- fsreqwrap access
- fsreqwrap readFile
- getaddrinforeq wrap
- getnameinforeq wrap
- pipe connect wrap
- query wrap
- pipewrap
- processwrap
- shutdown wrap
- tcpwrap
- udpwrap
- send wrap
- detailed signal wrap
- statwatcher
- timerwrap via setTimeout
- timerwrap via setInterval
- for Immediate
- http parser request
- http parser response
- connection via ssl server
- tls wrap
- write wrap
- ttywrap via readstream
- ttywrap via wriream
- zctx via zlib binding deflate

Embedder API:

-  async-event tests
  - one test looks at the happy paths
  - another ensures that in cases of events emitted in an order that
  doesn't make sense, the order is enforced by async hooks throwing a
  meaningful error
  - embedder enforcement tests are split up since async hook stack
  corruption now the process
  - therefore we launch a child and check for error output of the offending code

Additional tests:

- tests that show that we can enable/disable hooks inside their lifetime
events
- tests that verify the graph of resources triggering the creation of
other resources

Test Helpers:

- init-hooks:
  - returns one collector instance
  - when created an async hook is created and the lifetime events are
  registered to call the appropriate collector functions
  - the collector also exposes `enable` and `disable` functions which call
  through to the async hook

- hook checks:
  - checks invocations of life time hooks against the actual invocations
  that were collected
  - in some cases like `destroy` a min/max range of invocations can be
  supplied since in these cases the exact number is non-deterministic

- verify graph:
  - verifies the triggerIds of specific async resources are as expected,
  i.e. the creation of resources was triggered by the resource we expect
  - includes a printGraph function to generate easily readable test
  input for verify graph
  - both functions prune TickObjects to create less brittle and easier
  to understand tests

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@addaleax
Copy link
Member

Landed as #12892 in dd6e3f6...c68ebe8

@trevnorris
Copy link
Contributor Author

@AndreasMadsen I only just saw your latest list of comments. I'll get working on those.

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Also add checks in lib/tty.js and tests.

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
AsyncWrap will be going through many changes, and the old API will no
longer be used. So remove those tests that will no longer be useful.
They may be added back later using the new API, once fully implemented.

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
The number of ids is limited to 2^53-1 regardless of whether an int64_t
is used or not because JS is limited to a double. So to make conversion
simpler, track ids internally as a double. This will also make life
simpler when this is eventually exposed to JS via a Float64Array.

Rename AsyncWrap::get_uid() to AsyncWrap::get_id().

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
RandomBytes and PBKDF2 were using the same "generic" ObjectTemplate for
construction. Instead create one for each that is properly named.

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Instead of wrapping several providers into PROVIDER_CRYPTO, have them
all be named after their class. Rename other providers to also match
their class names. With the exception of Parser. Which is actually
HTTPParser.

Add PROVIDER_LENGTH to make better checks in WrapperInfo().

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Allow handles to retrieve their own uid's by adding a new method on the
FunctionTemplates. Implementation of these into all other classes will
come in a future commit.

Add the method AsyncWrap::GetAsyncId() to all inheriting class objects
so the uid of the handle can be retrieved from JS.

In all applicable locations, run ClearWrap() on the object holding the
pointer so that it never points to invalid memory and make sure Wrap()
is always run so the class pointer is correctly attached to the object
and can be retrieved so GetAsyncId() can be run.

In many places a class instance was not removing its own pointer from
object() in the destructor. This left an invalid pointer in the JS
object that could cause the application to segfault under certain
conditions.

Remove ClearWrap() from ReqWrap for continuity. The ReqWrap constructor
was not the one to call Wrap(), so it shouldn't be the one to call
ClearWrap().

Wrap() has been added to all constructors that inherit from AsyncWrap.
Normally it's the child most class. Except in the case of HandleWrap.
Which must be the constructor that runs Wrap() because the class pointer
is retrieved for certain calls and because other child classes have
multiple inheritance to pointer to the HandleWrap needs to be stored.

ClearWrap() has been placed in all FunctionTemplate constructors so that
no random values are returned when running getAsyncId(). ClearWrap() has
also been placed in all class destructors, except in those that use
MakeWeak() because the destructor will run during GC. Making the
object() inaccessible.

It could be simplified to where AsyncWrap sets the internal pointer,
then if an inheriting class needs one of it's own it could set it again.
But the inverse would need to be true also, where AsyncWrap then also
runs ClearWeak. Unforunately because some of the handles are cleaned up
during GC that's impossible. Also in the case of ReqWrap it runs Reset()
in the destructor, making the object() inaccessible. Meaning,
ClearWrap() must be run by the class that runs Wrap(). There's currently
no generalized way of taking care of this across all instances of
AsyncWrap.

I'd prefer that there be checks in there for these things, but haven't
found a way to place them that wouldn't be just as unreliable.

Add test that checks all resources that can run getAsyncId(). Would like
a way to enforce that any new classes that can also run getAsyncId() are
tested, but don't have one.

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Changes in the native code for the upcoming async_hooks module. These
have been separated to help with review and testing.

Changes include:

* Introduce an async id stack that tracks recursive calls into async
  execution contexts. For performance reasons the id stack is held as a
  double* and assigned to a Float64Array. If the stack grows too large
  it is then placed in it's own stack and replaced with a new double*.
  This should accommodate arbitrarily large stacks.

  I'm not especially happy with the complexity involved with this async
  id stack, but it's also the fastest and most full proof way of
  handling it that I have found.

* Add helper functions in Environment and AsyncWrap to work with the
  async id stack.

* Add AsyncWrap::Reset() to allow AsyncWrap instances that have been
  placed in a resource pool, instead of being released, to be
  reinitialized. AsyncWrap::AsyncWrap() also now uses Reset() for
  initialization.

* AsyncWrap* parent no longer needs to be passed via the constructor.

* Introduce Environment::AsyncHooks class to contain the needed native
  functionality. This includes the pointer to the async id stack, and
  array of v8::Eternal<v8::String>'s that hold the names of all
  providers, mechanisms for storing/retrieving the trigger id, etc.

* Introduce Environment::AsyncHooks::ExecScope as a way to track the
  current id and trigger id of function execution via RAII.

* If the user passes --abort-on-uncaught-exception then instead of
  throwing the application will print a stack trace and abort.

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Fill this commit messsage with more details about the change once all
changes are rebased.

* Add lib/async_hooks.js

* Add JS methods to AsyncWrap for handling the async id stack

* Introduce AsyncReset() so that JS functions can reset the id and again
  trigger the init hooks, allow AsyncWrap::Reset() to be called from JS
  via asyncReset().

* Add env variable to test additional things in test/common.js

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Implement async_hooks support in the following:

* fatalException handler
* process.nextTick
* Timers
* net/dgram/http

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Async wrap providers tested:

- crypto.randomBytes
- crypto.pbkdf2
- fs event wrap
- fsreqwrap access
- fsreqwrap readFile
- getaddrinforeq wrap
- getnameinforeq wrap
- pipe connect wrap
- query wrap
- pipewrap
- processwrap
- shutdown wrap
- tcpwrap
- udpwrap
- send wrap
- detailed signal wrap
- statwatcher
- timerwrap via setTimeout
- timerwrap via setInterval
- for Immediate
- http parser request
- http parser response
- connection via ssl server
- tls wrap
- write wrap
- ttywrap via readstream
- ttywrap via wriream
- zctx via zlib binding deflate

Embedder API:

-  async-event tests
  - one test looks at the happy paths
  - another ensures that in cases of events emitted in an order that
  doesn't make sense, the order is enforced by async hooks throwing a
  meaningful error
  - embedder enforcement tests are split up since async hook stack
  corruption now the process
  - therefore we launch a child and check for error output of the offending code

Additional tests:

- tests that show that we can enable/disable hooks inside their lifetime
events
- tests that verify the graph of resources triggering the creation of
other resources

Test Helpers:

- init-hooks:
  - returns one collector instance
  - when created an async hook is created and the lifetime events are
  registered to call the appropriate collector functions
  - the collector also exposes `enable` and `disable` functions which call
  through to the async hook

- hook checks:
  - checks invocations of life time hooks against the actual invocations
  that were collected
  - in some cases like `destroy` a min/max range of invocations can be
  supplied since in these cases the exact number is non-deterministic

- verify graph:
  - verifies the triggerIds of specific async resources are as expected,
  i.e. the creation of resources was triggered by the resource we expect
  - includes a printGraph function to generate easily readable test
  input for verify graph
  - both functions prune TickObjects to create less brittle and easier
  to understand tests

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants