src: use JS inheritance for `AsyncWrap` by addaleax · Pull Request #23094 · nodejs/node

addaleax · 2018-09-25T22:46:56Z

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 · 2018-09-25T22:46:58Z

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1021/pipeline

addaleax · 2018-09-25T22:47:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/17442/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1559/
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1560/

joyeecheung

YES!!!

Left a few comments, but nothing blocking.

src/async_wrap.cc

joyeecheung · 2018-09-26T03:11:58Z

src/env.h

  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)             \


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

src/process_wrap.cc

src/stream_wrap.cc

jasnell · 2018-09-28T23:07:21Z

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

addaleax · 2018-09-29T00:54:56Z

@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 · 2018-09-29T00:59:39Z

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 · 2018-09-29T01:05:33Z

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 · 2018-09-29T01:23:46Z

Hopefully ok then :)

addaleax · 2018-09-29T01:34:46Z

@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.

joyeecheung

Still LGTM

addaleax · 2018-09-30T15:25:05Z

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.)

danbev · 2018-10-02T08:47:13Z

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

addaleax · 2018-10-03T03:42:49Z

Windows again: https://ci.nodejs.org/job/node-test-commit-windows-fanned/21222/

addaleax · 2018-10-03T03:50:31Z

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 · 2018-10-03T09:13:07Z

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 · 2018-10-03T14:35:12Z

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 · 2018-10-03T16:53:54Z

… and again, Windows: https://ci.nodejs.org/job/node-test-commit-windows-fanned/21229/

addaleax · 2018-10-03T20:57:16Z

Landed in d527dde

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 <joyeec9h3@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <joyeec9h3@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <joyeec9h3@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <joyeec9h3@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

src: use JS inheritance for AsyncWrap

b2eeb79

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 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

joyeecheung approved these changes Sep 26, 2018

View reviewed changes

addaleax added 2 commits September 27, 2018 08:26

fixup! src: use JS inheritance for AsyncWrap

0b866b6

fixup! src: use JS inheritance for AsyncWrap

70b828b

joyeecheung approved these changes Sep 29, 2018

View reviewed changes

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 approved these changes Oct 2, 2018

View reviewed changes

joyeecheung mentioned this pull request Oct 2, 2018

CI failures: 20180927-20181002 nodejs/reliability#18

Closed

jasnell approved these changes Oct 2, 2018

View reviewed changes

addaleax closed this Oct 3, 2018

addaleax deleted the __proto__ branch October 3, 2018 20:57

addaleax added the backported-to-v10.x label Oct 3, 2018

addaleax mentioned this pull request Oct 3, 2018

(v10.x backport) src: use JS inheritance for AsyncWrap #23247

Closed

targos mentioned this pull request Oct 7, 2018

Release proposal: v10.12.0 #23313

Merged

Uh oh!

Conversation

addaleax commented Sep 25, 2018

Checklist

Uh oh!

nodejs-github-bot commented Sep 25, 2018

Uh oh!

addaleax commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joyeecheung Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jasnell commented Sep 28, 2018

Uh oh!

addaleax commented Sep 29, 2018

Uh oh!

jasnell commented Sep 29, 2018

Uh oh!

addaleax commented Sep 29, 2018

Uh oh!

jasnell commented Sep 29, 2018

Uh oh!

addaleax commented Sep 29, 2018

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 30, 2018

Uh oh!

danbev commented Oct 2, 2018

Uh oh!

addaleax commented Oct 3, 2018

Uh oh!

addaleax commented Oct 3, 2018

Uh oh!

joyeecheung commented Oct 3, 2018

Uh oh!

addaleax commented Oct 3, 2018

Uh oh!

addaleax commented Oct 3, 2018

Uh oh!

addaleax commented Oct 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

addaleax commented Sep 25, 2018 •

edited

Loading