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

src: use JS inheritance for AsyncWrap #23094

Closed
wants to merge 3 commits into from
Closed

Conversation

addaleax
Copy link
Member

For all classes descending from AsyncWrap, use JS inheritance
instead of manually adding methods to the individual classes.

This allows cleanup of some code around transferring handles
over IPC.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

For all classes descending from `AsyncWrap`, use JS inheritance
instead of manually adding methods to the individual classes.

This allows cleanup of some code around transferring handles
over IPC.
@nodejs-github-bot nodejs-github-bot 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 Sep 25, 2018
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

YES!!!

Left a few comments, but nothing blocking.

src/async_wrap.cc Show resolved Hide resolved
src/env.h Outdated
@@ -319,7 +319,8 @@ struct PackageConfig {
V(async_hooks_destroy_function, v8::Function) \
V(async_hooks_init_function, v8::Function) \
V(async_hooks_promise_resolve_function, v8::Function) \
V(async_wrap_constructor_template, v8::FunctionTemplate) \
V(async_wrap_object_constructor_template, v8::FunctionTemplate) \
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use ctor for this name as well, for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

src/process_wrap.cc Show resolved Hide resolved
src/stream_wrap.cc Show resolved Hide resolved
@jasnell
Copy link
Member

jasnell commented Sep 28, 2018

Definitely a good idea, I think, but is this going to be semver-major?

@addaleax
Copy link
Member Author

@jasnell If necessary, I would be okay with that, but could you explain why this is semver-major? It doesn’t touch public API or remove methods, it just moves them to a different position on the prototype chain (of internal objects).

@jasnell
Copy link
Member

jasnell commented Sep 29, 2018

Largely defensive. It shouldn't be breaking but as we've seen many times, it pays to be careful. At the very least we need a citgm run to be sure

@addaleax
Copy link
Member Author

I ran CITGM – there are a lot of failures because of #23122, but in the diff between the run here and the master run, I didn’t find anything that would point to issues with this PR.

@jasnell
Copy link
Member

jasnell commented Sep 29, 2018

Hopefully ok then :)

@addaleax
Copy link
Member Author

@jasnell I think this will require a backport PR for v10.x anyway … let’s run CITGM on that as well, that should give a much clearer picture.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Still LGTM

@addaleax
Copy link
Member Author

New CI: https://ci.nodejs.org/job/node-test-pull-request/17517/

(This still needs to wait for a second approval, or two more days.)

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 30, 2018
@danbev
Copy link
Contributor

danbev commented Oct 2, 2018

Re-run of node-test-commit-aix.
Re-run of node-test-commit-windows-fanned.
Re-run of node-test-commit-linux,

@addaleax
Copy link
Member Author

addaleax commented Oct 3, 2018

@addaleax
Copy link
Member Author

addaleax commented Oct 3, 2018

Btw, @joyeecheung … just thinking out loud: The most natural way to do mirror the C++ multiple inheritance might be to create a second native JS class (e.g. StreamBase) and have getters so that from any given object, both are accessible? For example, if handle is a native TCP handle, handle.stream instanceof StreamBase && handle.stream.wrap === handle (the name wrap is probably to be replaced with something better)?

That sounds semver-major to me, though, and I still have some native stream refactoring that I’d like to do, so maybe I’d keep that approach in mind and we could implement it late in the Node 11 release cycle?

@joyeecheung
Copy link
Member

create a second native JS class (e.g. StreamBase) and have getters so that from any given object, both are accessible?

Isn't that kind of like StreamBase::AddMethods done in JS land with declarative accessors? But come to think of it, if we are going to do multiple inheritance in C++, then it's probably better to keep doing inheritance in JS instead of going with inheritance in C++ while with composition in JS - that seems more confusing than doing multiple inheritance on both sides.

(Or..maybe we should do it the other way around, why does LibuvStreamWrap have multiple inheritance in the first place?)
(either way I believe it's going to be semver-major so yeah, +1 to do it later)

@addaleax
Copy link
Member Author

addaleax commented Oct 3, 2018

Or..maybe we should do it the other way around, why does LibuvStreamWrap have multiple inheritance in the first place?

Because it’s both a class that wraps a libuv handle, and is a stream at the same time – it does make sense to use multiple inheritance here…

@addaleax
Copy link
Member Author

addaleax commented Oct 3, 2018

@addaleax
Copy link
Member Author

addaleax commented Oct 3, 2018

Landed in d527dde

@addaleax addaleax closed this Oct 3, 2018
@addaleax addaleax deleted the __proto__ branch October 3, 2018 20:57
addaleax added a commit that referenced this pull request Oct 3, 2018
For all classes descending from `AsyncWrap`, use JS inheritance
instead of manually adding methods to the individual classes.

This allows cleanup of some code around transferring handles
over IPC.

PR-URL: #23094
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Oct 3, 2018
For all classes descending from `AsyncWrap`, use JS inheritance
instead of manually adding methods to the individual classes.

This allows cleanup of some code around transferring handles
over IPC.

PR-URL: nodejs#23094
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 7, 2018
For all classes descending from `AsyncWrap`, use JS inheritance
instead of manually adding methods to the individual classes.

This allows cleanup of some code around transferring handles
over IPC.

Backport-PR-URL: #23247
PR-URL: #23094
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 7, 2018
For all classes descending from `AsyncWrap`, use JS inheritance
instead of manually adding methods to the individual classes.

This allows cleanup of some code around transferring handles
over IPC.

Backport-PR-URL: #23247
PR-URL: #23094
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
For all classes descending from `AsyncWrap`, use JS inheritance
instead of manually adding methods to the individual classes.

This allows cleanup of some code around transferring handles
over IPC.

PR-URL: #23094
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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