Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Remove all AsyncListener JS code #8110

Closed
wants to merge 9 commits into from
Closed

Remove all AsyncListener JS code #8110

wants to merge 9 commits into from

Conversation

trevnorris
Copy link

Squashed for simplicity. There are also changes in the naming of some of the native API's so what it's called actually reflects what is going on. Also added a few TODO's, etc.

Review, comment, and if necessary I'll break up the commits into relevant segments.

Also, didn't add tjfontaine@6a74d6c to the changes. If that conditional is hit then there's a bug in the code. I'd rather add an ASSERT() or the like that can be reported instead of silently ignored.

/cc @tjfontaine @indutny @orangemocha

@tjfontaine tjfontaine added in progress and removed pr labels Aug 8, 2014
@trevnorris trevnorris self-assigned this Aug 8, 2014
@trevnorris trevnorris added this to the v0.12 milestone Aug 8, 2014
v8::TryCatch try_catch;
try_catch.SetVerbose(true);

v8::Local<v8::Value> val = object.As<v8::Value>();
env->async_listener_run_function()->Call(env->process_object(), 1, &val);
if (parent_has_async_queue) {
Copy link
Member

Choose a reason for hiding this comment

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

So this is technically leaving us a way to hook this up, right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. This check is for specific cases like incoming TCP connections. Because receiving the connection is asynchronous, the call stack to the server is destroyed. Hence the call to env->set_async_wrap_parent_class<T>() in tcp_wrap just before the instantiation of the JS object for the incoming connection. That way the connection can be traced back to the server that is the reason the incoming connection exists.

Now, to put all that into code comments...

@indutny
Copy link
Member

indutny commented Aug 8, 2014

I don't know all details, but it LGTM

@aredridel
Copy link

Any tests for the new functionality?

@trevnorris
Copy link
Author

The hooks will remain accessible via process.binding('async_listener'), and
the added functionality will live in an external module.

@tjfontaine, basic tests could be added to make sure functionality to the
binding is working correctly. Don't think that should prevent this PR
(which is holding up the v0.12 release) from landing, but I'll work on
those.

@loyd loyd mentioned this pull request Aug 11, 2014
@orangemocha
Copy link
Contributor

Passed Windows build and tests so I give you my +1

@trevnorris
Copy link
Author

For the sake of #8090, I've added an additional commit that gives all AsyncWrap instances a unique id. The change is minimal at best, but takes a different approach than that in the other PR.

/cc @bnoordhuis

@kraman
Copy link

kraman commented Aug 12, 2014

@trevnorris I find @bnoordhuis's af669a5 commit to also be very useful in tracking the lifecycle of a handles. I am using it in Zones to do the same. Since this is an addition to the AsyncWrap, can you please take a look and see if it makes sense to add a similar commit to this PR?

@trevnorris
Copy link
Author

@kraman The specific change I included is how the id of the AsyncWrap instance is accessed. It wasn't meant to replace the additional functionality included in @bnoordhuis' PR. That is distinct from AsyncWrap directly, and should be in another PR.

@kraman
Copy link

kraman commented Aug 12, 2014

@trevnorris ok, thanks. I'll look into rebasing @bnoordhuis's PR unless he gets to it first

@bnoordhuis
Copy link
Member

@kraman I would suggest holding off until this PR lands.

@bnoordhuis
Copy link
Member

@trevnorris I think there is at least one place where AsyncWrap::AddMethods() isn't getting applied (to fs request objects in node_file.cc) and there may be more.

Maybe it's better to make everything inherit from a common ObjectTemplate. That would also make it easier to protect against invocations with the wrong holder (req.asyncId.call({}).)

@trevnorris
Copy link
Author

@bnoordhuis

I think there is at least one place where AsyncWrap::AddMethods() isn't getting applied

Correct. Now that you bring it up, I remember now that the first method was only added to the classes where it was necessary to be called from JS. Forgot about that.

Maybe it's better to make everything inherit from a common ObjectTemplate.

I like the idea, but have an idea how to implement this? Can't think of how to create a "base" ObjectTemplate and have other inheriting classes inherit from that.

@bnoordhuis
Copy link
Member

I like the idea, but have an idea how to implement this? Can't think of how to create a "base" ObjectTemplate and have other inheriting classes inherit from that.

It probably requires a fair amount of rework so I was hoping you'd volunteer. :-)

What I think needs to happen is that you have a single FunctionTemplate with a configured InstanceTemplate (that's the ObjectTemplate) for constructing plain request and handle objects. Utility methods like .asyncId() can then go on the PrototypeTemplate.

If you need to specialize for a request or handle type, you create a new FunctionTemplate and make it inherit from the main one with FunctionTemplate::Inherit(). Prototypal inheritance in JS would also work but Inherit() might be more efficient.

@trevnorris
Copy link
Author

@bnoordhuis Pretty much anything that inherits from ReqWrap doesn't receive the added methods. This is because it expects an object to be passed in when instantiated. Take for example how it's done in lib/dgram.js:

      var req = { buffer: buffer, length: length };  // Keep reference alive.
      if (callback) {
        req.callback = callback;
        req.oncomplete = afterSend;
      }
      var err = self._handle.send(req,
                                  buffer,
                                  // ...

I'm sure you see what's going on.

Right now I'm trying to think of a quicker way to fix this so it can be merged and v0.12 can be closer to being shipped. Then finish the more "elegant" way later. It'll come at a performance hit. Not sure how much. Going to give it a go and see how much of a difference it'll make.

@trevnorris
Copy link
Author

@bnoordhuis I'm curious if the API itself isn't an issue to make asyncId() useful. For example:

var dgram = require('dgram');
var b = new Buffer('hi\n');
var s = dgram.createSocket('udp4');
s.bind(8000);

// This returns nothing.
s.send(b, 0, b.length, 8000, 'localhost', function cb() {
  // Access to asyncId() is only available here.
  console.log(this.asyncId());
});

You'll see by my latest commits how I'm planning on addressing the issue in the interim, but the fact that the request id isn't accessible until after the request is made sort of defeats the need.

As for performance, specifically the dgram case, there is no performance hit. In fact, it's a hair faster. Statistically pretty much within the margin of error, but still is consistently faster.

piscisaureus pushed a commit to piscisaureus/node2 that referenced this pull request Dec 9, 2014
Async Listener was the name of the user-facing JS API, and is being
completely removed. Instead low level hooks directly into the mechanism
that AL used will be introduced in a future commit.

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
piscisaureus pushed a commit to piscisaureus/node2 that referenced this pull request Dec 9, 2014
MakeCallback is too large a function to be inlined. Likewise, only
having header files will not allow for any part of AsyncWrap to be
exposed cleanly via NODE_MODULE_CONTEXT_AWARE_BUILTIN().

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
piscisaureus pushed a commit to piscisaureus/node2 that referenced this pull request Dec 9, 2014
C++ won't deoptimize like JS if specific conditional branches are
sporadically met in the future. Combined with the amount of code
duplication removal and simplified maintenance complexity, it makes more
sense to merge MakeCallback and MakeDomainCallback.

Additionally, type casting in V8 before verifying what that type is will
cause V8 to abort in debug mode if that type isn't what was expected.
Fix this by first checking the v8::Value before casting.

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
piscisaureus pushed a commit to piscisaureus/node2 that referenced this pull request Dec 9, 2014
An edge case could occur when the setImmediate() in _fatalException()
would fire before the timers module had been loaded globally, causing
Node to crash.

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
piscisaureus pushed a commit to piscisaureus/node2 that referenced this pull request Dec 9, 2014
Instead of simply creating a new v8::Object to contain the connection
information, instantiate a new instance of a FunctionTemplate. This will
allow future improvements for debugging and performance probes.

Additionally, the "provider" argument in the ReqWrap constructor is no
longer optional.

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
piscisaureus pushed a commit to piscisaureus/node2 that referenced this pull request Dec 9, 2014
The template class information is received via the type of the first
argument. So there is no need to use Wrap<T>(handle).

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
piscisaureus pushed a commit to piscisaureus/node2 that referenced this pull request Dec 9, 2014
Expose basic hooks for AsyncWrap via the async_wrap binding. Right now
only the PROVIDER types are exposed. This is a preliminary step before
more functionality is added.

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
piscisaureus pushed a commit to piscisaureus/node2 that referenced this pull request Dec 9, 2014
When instantiating a new AsyncWrap allow the parent AsyncWrap to be
passed. This is useful for cases like TCP incoming connections, so the
connection can be tied to the server receiving the connection.

Because the current architecture instantiates the *Wrap inside a
v8::FunctionCallback, the parent pointer is currently wrapped inside a
new v8::External every time and passed as an argument. This adds ~80ns
to instantiation time.

A future optimization would be to add the v8::External as the data field
when creating the v8::FunctionTemplate, change the pointer just before
making the call then NULL'ing it out afterwards. This adds enough code
complexity that it will not be attempted until the current approach
demonstrates it is a bottle neck.

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
piscisaureus pushed a commit to piscisaureus/node2 that referenced this pull request Dec 9, 2014
Call a user-defined callback at specific points in the lifetime of an
asynchronous event. Which are on instantiation, just before/after the
callback has been run.

**If any of these callbacks throws an exception, there is no forgiveness
or recovery. A message will be displayed and a core file dumped.**

Currently these only tie into AsyncWrap, meaning no call to a hook
callback will be made for timers or process.nextTick() events. Though
those will be added in a future commit.

Here are a few notes on how to make the hooks work:

- The "this" of all event hook callbacks is the request object.

- The zero field (kCallInitHook) of the flags object passed to
  setupHooks() must be set != 0 before the init callback will be called.

- kCallInitHook only affects the calling of the init callback. If the
  request object has been run through the create callback it will always
  run the before/after callbacks. Regardless of kCallInitHook.

- In the init callback the property "_asyncQueue" must be attached to
  the request object. e.g.

  function initHook() {
    this._asyncQueue = {};
  }

- DO NOT inspect the properties of the object in the init callback.
  Since the object is in the middle of being instantiated there are some
  cases when a getter is not complete, and doing so will cause Node to
  crash.

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
trevnorris added a commit to nodejs/node that referenced this pull request Dec 9, 2014
Async Listener was the name of the user-facing JS API, and is being
completely removed. Instead low level hooks directly into the mechanism
that AL used will be introduced in a future commit.

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
trevnorris added a commit to nodejs/node that referenced this pull request Dec 9, 2014
MakeCallback is too large a function to be inlined. Likewise, only
having header files will not allow for any part of AsyncWrap to be
exposed cleanly via NODE_MODULE_CONTEXT_AWARE_BUILTIN().

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
trevnorris added a commit to nodejs/node that referenced this pull request Dec 9, 2014
C++ won't deoptimize like JS if specific conditional branches are
sporadically met in the future. Combined with the amount of code
duplication removal and simplified maintenance complexity, it makes more
sense to merge MakeCallback and MakeDomainCallback.

Additionally, type casting in V8 before verifying what that type is will
cause V8 to abort in debug mode if that type isn't what was expected.
Fix this by first checking the v8::Value before casting.

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
trevnorris added a commit to nodejs/node that referenced this pull request Dec 9, 2014
An edge case could occur when the setImmediate() in _fatalException()
would fire before the timers module had been loaded globally, causing
Node to crash.

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
trevnorris added a commit to nodejs/node that referenced this pull request Dec 9, 2014
Instead of simply creating a new v8::Object to contain the connection
information, instantiate a new instance of a FunctionTemplate. This will
allow future improvements for debugging and performance probes.

Additionally, the "provider" argument in the ReqWrap constructor is no
longer optional.

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
trevnorris added a commit to nodejs/node that referenced this pull request Dec 9, 2014
The template class information is received via the type of the first
argument. So there is no need to use Wrap<T>(handle).

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
trevnorris added a commit to nodejs/node that referenced this pull request Dec 9, 2014
Expose basic hooks for AsyncWrap via the async_wrap binding. Right now
only the PROVIDER types are exposed. This is a preliminary step before
more functionality is added.

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
trevnorris added a commit to nodejs/node that referenced this pull request Dec 9, 2014
When instantiating a new AsyncWrap allow the parent AsyncWrap to be
passed. This is useful for cases like TCP incoming connections, so the
connection can be tied to the server receiving the connection.

Because the current architecture instantiates the *Wrap inside a
v8::FunctionCallback, the parent pointer is currently wrapped inside a
new v8::External every time and passed as an argument. This adds ~80ns
to instantiation time.

A future optimization would be to add the v8::External as the data field
when creating the v8::FunctionTemplate, change the pointer just before
making the call then NULL'ing it out afterwards. This adds enough code
complexity that it will not be attempted until the current approach
demonstrates it is a bottle neck.

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
trevnorris added a commit to nodejs/node that referenced this pull request Dec 9, 2014
Call a user-defined callback at specific points in the lifetime of an
asynchronous event. Which are on instantiation, just before/after the
callback has been run.

**If any of these callbacks throws an exception, there is no forgiveness
or recovery. A message will be displayed and a core file dumped.**

Currently these only tie into AsyncWrap, meaning no call to a hook
callback will be made for timers or process.nextTick() events. Though
those will be added in a future commit.

Here are a few notes on how to make the hooks work:

- The "this" of all event hook callbacks is the request object.

- The zero field (kCallInitHook) of the flags object passed to
  setupHooks() must be set != 0 before the init callback will be called.

- kCallInitHook only affects the calling of the init callback. If the
  request object has been run through the create callback it will always
  run the before/after callbacks. Regardless of kCallInitHook.

- In the init callback the property "_asyncQueue" must be attached to
  the request object. e.g.

  function initHook() {
    this._asyncQueue = {};
  }

- DO NOT inspect the properties of the object in the init callback.
  Since the object is in the middle of being instantiated there are some
  cases when a getter is not complete, and doing so will cause Node to
  crash.

PR-URL: nodejs/node-v0.x-archive#8110
Signed-off-by: Trevor Norris <[email protected]>
Reviewed-by: Fedor Indutny <[email protected]>
Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
@kenany kenany mentioned this pull request Jan 23, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants