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: make native API public #3504

Closed
wants to merge 1 commit into from

Conversation

trevnorris
Copy link
Contributor

Rough attempt to allow AsyncWrap to be include'able in a module.

R=@bnoordhuis
R=@indutny

This is a WIP, and am only posting for critique on the approach. Tests still need to be added, and one of them fixed.

@trevnorris trevnorris added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 23, 2015
ProviderType provider,
AsyncWrap* parent)
: BaseObject(isolate, object), bits_(static_cast<uint32_t>(provider) << 1) {
CHECK_NE(provider, PROVIDER_NONE);
Copy link
Member

Choose a reason for hiding this comment

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

I would just have a separate Init function or whatever, to avoid duplication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. I left it this way wanting to just get something building. I'll fix it up. :)

@indutny
Copy link
Member

indutny commented Oct 24, 2015

I think it is a bit duplicated, but otherwise looking pretty good!

@AndreasMadsen
Copy link
Member

Is this meant as an alternative to setting the _asyncQueue property on the this object when using MakeCallback?

@trevnorris
Copy link
Contributor Author

@AndreasMadsen I've been pondering that. Do you use that logic? Nothing in node currently does, and if there's no usage I was thinking I'd simply remove the logic all together. Since @indutny is requesting we make AsyncWrap public anyway users could then hook into that mechanism instead.

@jasnell jasnell added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Oct 26, 2015
@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

I forgot... we're not labeling the asyncwrap stuff yet ;-)

@trevnorris trevnorris force-pushed the make-async-wrap-public branch from 82015ad to 8556434 Compare October 26, 2015 17:46
@trevnorris
Copy link
Contributor Author

@indutny My attempt at de-duplicating the code. Feel like there's a better way, but nothing is apparent.

@jasnell Hopefully soon. :)

@AndreasMadsen
Copy link
Member

@trevnorris No I don't use the _asyncQueue property and I haven't seen any module that does. I think it is an odd design, since they init hook has to be called manually anyway (in most usecases). And if that is done, you are so deep into AsyncWrap that you might as well call the pre and post hooks yourself.

I would much rather see the AsyncWrap class being used properly and perhaps integrated into the Nan:: Callback class or similar.

ProviderType provider,
AsyncWrap* parent)
: BaseObject(isolate, object), bits_(static_cast<uint32_t>(provider) << 1) {
Environment* env = Environment::GetCurrent(isolate);
Copy link
Member

Choose a reason for hiding this comment

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

Exactly!

@trevnorris trevnorris force-pushed the make-async-wrap-public branch from 8556434 to 719d647 Compare October 26, 2015 20:05
Rough attempt to allow AsyncWrap to be include'able in a module.
@trevnorris trevnorris force-pushed the make-async-wrap-public branch from 719d647 to 238be1c Compare October 26, 2015 20:24
@indutny
Copy link
Member

indutny commented Oct 26, 2015

LGTM, but I would like to have @bnoordhuis review too

Local<Object> object,
ProviderType provider,
AsyncWrap* parent)
: BaseObject(env, object), bits_(static_cast<uint32_t>(provider) << 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make bits_ a uintptr_t so there is room to change it to a pointer without breaking ABI.

@bnoordhuis
Copy link
Member

It's not entirely clear to me why the changes to src/base-object* are necessary. Were you planning on making those public as well?

@trevnorris
Copy link
Contributor Author

@bnoordhuis Specifically because of

inline BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> handle)
    : handle_(env->isolate(), handle),
      env_(env) {

Which requires including env.h and env-inl.h. Neither of which can be included by the user. But since async-wrap-inl.h included base-object.h I was still getting build errors. Though, by moving everything out of async-wrap-inl.h to async-wrap.cc that may resolve the issue by itself.

@AndreasMadsen
Copy link
Member

@trevnorris What is the status?

@trevnorris
Copy link
Contributor Author

From conversation I had with @bnoordhuis:

<bnoordhuis> remember i [sic] wrote base-object.h so we could stop using ObjectWrap in core, which we couldn't change easily because it was public
<trevnorris> hah. true.
<trevnorris> i'm just experimenting w/ making AsyncWrap public at indutny's request. it's nothing I have my mind set on.
<bnoordhuis> i'd take the unix file descriptor approach: give the user an integer that represents the object and hide everything else
<bnoordhuis> don't expose complex api, it always ends in tears
<trevnorris> think the reason is indutny wants to expose JSStream as a public native API, which depends on AsyncWrap.
<bnoordhuis> exposing JSStream seems like it's guaranteed to end in tears, only question is whose tears
<bnoordhuis> mine, at the very least

@indutny
Copy link
Member

indutny commented Jan 12, 2016

😢

@AndreasMadsen
Copy link
Member

@trevnorris Another reason for making AsyncWrap public was so native addons could integrate with AsyncWrap. But I suspect it can be done with a simple API and integers.

@trevnorris
Copy link
Contributor Author

@AndreasMadsen I hypothetically like the idea of doing this, and will continue to think about another way this could be done more elegantly.

@trevnorris trevnorris closed this Mar 28, 2016
@trevnorris trevnorris deleted the make-async-wrap-public branch March 28, 2016 22:27
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++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants