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_wrap: WIP/Experiment, init/destroy HTTPParser on each use #5573

Closed
wants to merge 2 commits into from

Conversation

omsmith
Copy link
Contributor

@omsmith omsmith commented Mar 5, 2016

Wasn't sure the best place to have this discussion, so I went with opening a PR and figure it can be closed.

Been working with AsyncWrap a bit and it was very cool to see a7e49c8 land the other day :). Was playing with the following, which I know is probably horrible, of firing the init and destroy hooks per usage of the HTTPParser, giving a clear parent/child relationship per-request, while still being able to reuse the actually instances.

Just wanted to get thoughts on the concept, @trevnorris

@mscdex mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 5, 2016
@trevnorris
Copy link
Contributor

Excellent. This is on my todo list. Here are a few things that need to be addressed:

  1. The destructor can't be removed. Otherwise you need to run DestroyAsyncWrap() in every child destructor. Not a maintainable solution.
  2. Given the above, it's necessary to alert the constructor that it should not run the init callback. I have not figured out a satisfactory implementation. Feel free to experiment.
  3. If the constructor is explicitly told not to run run init that should be stored on the class instance so the destructor can know not to call the destroy callback. Since it should also be manually run in the same case.

The other use case for this are timers. Multiple JS timer instances are stored on a single TimerWrap. Meaning the async wrap callbacks must be manually triggered.

@omsmith
Copy link
Contributor Author

omsmith commented Mar 22, 2016

Thanks for following up - good to hear it's not completely off base.

The destructor is still in place, although the diff makes the change a little non-obvious.

Good call about not initting/destroying, while consumers should be able to manage the handle reuse, two init callbacks "back-to-back" for the same handle is reasonably poor behaviour. Sounds like we just need to figure out a nice way to indicate "don't manage lifecycle events" - I'll play around with it.

omsmith added 2 commits March 22, 2016 14:03
Some objects, such as HttpParser or TimerWrap, are repurposed across
distinct usages of them to avoid construction/destruction churn. As
such, while calling init/destroy callbacks on construction and destruction
makes sense from an object lifecycle standpoint, it doesn't for what
AsyncWrap is actually trying to achieve.

This allows such objects to manually handle the init/destroy timing.
@omsmith omsmith force-pushed the reinit-httpparser-asyncwrap branch from be1578b to 64082db Compare March 22, 2016 18:16
@omsmith
Copy link
Contributor Author

omsmith commented Mar 22, 2016

Updated with super-basic flagging off init/destroy within constructor/destructor. Maybe not the nicest interface - and it would probably need to be extended up to HandleWrap as well, which isn't very nice.

(As is, parallel/test-async-wrap-check-providers fails, because construction of the parser doesn't call init)

@@ -457,6 +457,7 @@ class Parser : public AsyncWrap {
// Should always be called from the same context.
CHECK_EQ(env, parser->env());
parser->Init(type);
parser->InitAsyncWrap(env, args.This(), AsyncWrap::PROVIDER_HTTPPARSER);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would we have some way to access parent at this point, that we could start providing it through the Init?

Presently, if you disable async_wrap, you don't see the HTTPParser come through, because our parent is null. If we could tie it to the parent server, it would respect the rules a bit better.

shrug

@AndreasMadsen
Copy link
Member

Should the uid not be incremented on each InitAsyncWrap call?

@trevnorris
Copy link
Contributor

@AndreasMadsen You're correct. I have an implementation of this in a local branch while I work out the timer implementation details. It's basically asycn_wrap.new_uid() which returns the new uid and increments the internal uid counter.

@omsmith There are several issues with how HTTPParser works. One of them I'm currently working out is how to propagate the connection to the HTTPParser instance.

>>> id: 1 TCPWRAP
>>> id: 2 TCPWRAP
before: 1             <-- HTTPPARSER happens in scope of server
>>> id: 3 TIMERWRAP
>>> id: 4 HTTPPARSER
after: 1
before: 4
>>> id: 5 CRYPTO
>>> id: 6 TIMERWRAP
after: 4

This shows that the correct parent (i.e. the connection) is not active and cannot propagate to the parser. These issues are all loosely related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants