src: extracting OnConnection from pipe_wrap and tcp_wrap #7547

danbev · 2016-07-05T19:20:19Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

Description of change

One of the issues in #4641 concerns OnConnection in pipe_wrap and
tcp_wrap which are very similar with some minor difference in how
they are coded. This commit extracts OnConnection so both these
classes can share the same implementation.

One of the issues in nodejs#4641 concerns OnConnection in pipe_wrap and tcp_wrap which are very similar with some minor difference in how they are coded. This commit extracts OnConnection so both these classes can share the same implementation.

Fishrock123 · 2016-07-07T08:36:48Z

Thanks for the PR! Here is a CI test run: https://ci.nodejs.org/job/node-test-pull-request/3199/

Fishrock123 · 2016-07-07T11:01:49Z

Seems fine to me, tests are passing. i'll let some more C++ -y folks sign-off though.

cc @trevnorris / @addaleax / @bnoordhuis / etc

addaleax · 2016-07-07T11:18:36Z

src/connection_wrap.h

+
+class ConnectionWrap {
+ protected:
+  template<typename WrapType, typename UVType>


It might be nicer to have the templateing done on the class itself, class A : public B<A> { … }; isn’t that uncommon of a pattern + it would allow lifting the handle_ to ConnectionWrap as UVType handle_; (I think?).

Ah, that does sound better. Let me give that a try and see what you think. Thanks

I originally just wanted to move the handle_ member from tcp_wrap and pipe_wrap without changing its name. But I ran into an issue which I believe is because tcp_wrap and pipe_wrap now extend ConnectionWrap. Both classes are derived from StreamWrap which has an inheritance tree that looks like this: class PipeWrap : public StreamWrap, ConnectionWrap<PipeWrap, uv_pipe_t> class StreamWrap : public HandleWrap, public StreamBase class HandleWrap : public AsyncWrap class AsyncWrap : public BaseObject BaseObject has the following private member: v8::Persistent<v8::Object> handle_; The compiler complains that 'handle_' is found in multiple base classes of different types: ../src/pipe_wrap.cc:130:50: error: member 'handle_' found in multiple base classes of different types reinterpret_cast<uv_stream_t*>(&handle_), ^ ../src/base-object.h:47:30: note: member found by ambiguous name lookup v8::Persistent<v8::Object> handle_; It turns out that name lookup is performed before access rules are considered. C++ standard section 3.4: "The access rules (Clause 11) are considered only once name lookup and function overload resolution (if applicable) have succeeded." It is possible to be explicit about the type you want using the resolution operator ::. So we could use ConnectionWrap::handle_ to tell the compiler which type we want. But going down this route still caused changes to a number of files and I think the code is somewhat clearer using a different name for the handle, and there is no confusion with the handle_ member in BaseObject. Being fairly new to C++ and to the project there this is probably a gap in my knowledge about the best way to solve this. Looking forward to hear about other options/ideas for this.

addaleax · 2016-07-08T10:47:26Z

src/connection_wrap.cc

+template<typename WrapType, typename UVType>
+UVType* ConnectionWrap<WrapType, UVType>::UVHandle() {
+  return &uvhandle_;
+}


It might be easier to define UVHandle() directly in the header where it’s defined as an inline function?

Sounds, I'll fix that.

addaleax · 2016-07-08T10:50:31Z

That handle_ -> uvhandle_ change touches a lot of code… was there anything that made it necessary?

danbev · 2016-07-08T10:53:52Z

That handle_ -> uvhandle_ change touches a lot of code… was there anything that made it necessary?

Yeah, it is unfortunate. I've added a comment about this in 34d5abc message.

addaleax · 2016-07-08T10:58:33Z

Yeah, it is unfortunate. I've added a comment about this in 34d5abc message.

Sorry, right. Maybe renaming handle_ in BaseObject is easier though, because the changes would be more localized + affect only the name of a private member?

danbev · 2016-07-08T11:05:33Z

Maybe renaming handle_ in BaseObject is easier though, because the changes would be more localized + affect only the name of a private member?

That sounds much better. I was not sure if changing BaseObject was "off limits" or not. I'll take a stab at this and your other comments this evening or during the weekend. I appreciate your feedback and help, thanks.

trevnorris · 2016-07-08T20:09:00Z

src/connection_wrap.cc

+void ConnectionWrap<WrapType, UVType>::OnConnection(uv_stream_t* handle,
+                                                    int status) {
+  WrapType* wrap_data = static_cast<WrapType*>(handle->data);
+  CHECK_EQ(&wrap_data->uvhandle_, reinterpret_cast<UVType*>(handle));


Mind adding a CHECK_NE(wrap_data, nullptr) just above this?

No problems, I'll add this.

trevnorris · 2016-07-08T20:12:08Z

Side note, with this change we can do something about the following in handle_wrap.h:

  // Using double underscore due to handle_ member in tcp_wrap. Probably
  // tcp_wrap should rename it's member to 'handle'.
  uv_handle_t* const handle__;

bnoordhuis · 2016-07-09T11:08:00Z

src/connection_wrap.cc

+using v8::Value;
+
+
+template<typename WrapType, typename UVType>


Tiny style nit: space before <.

I'll correct this.

addaleax · 2016-07-09T12:53:02Z

src/connection_wrap.cc

+
+template uv_pipe_t* ConnectionWrap<PipeWrap, uv_pipe_t>::UVHandle();
+
+template uv_tcp_t* ConnectionWrap<TCPWrap, uv_tcp_t>::UVHandle();


I think these two can be dropped now?

Ah yes, I'll remove them.

addaleax · 2016-07-25T08:00:23Z

Landed in 4663393, thank you!

One of the issues in #4641 concerns OnConnection in pipe_wrap and tcp_wrap which are very similar with some minor difference in how they are coded. This commit extracts OnConnection so both these classes can share the same implementation. PR-URL: #7547 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jbergstroem · 2016-07-25T18:42:41Z

These test failures popped up pretty recently, I fear they might be related. Look at centos5/6 and ubuntu12 bots: https://ci.nodejs.org/job/node-test-commit-linux/4316/

Trott · 2016-07-25T18:55:21Z

It looks like 85af1a6 was added after the CI run. So, with no other information, I'd guess that tiny change is the culprit...

Sure enough, undoing that one tiny change fixes it:

https://ci.nodejs.org/job/node-test-commit-linux/4320/

85af1a6 was added (squashed into another commit) without running through CI. Unfortunately, it breaks some of the CI builds, notably on three of the CentOS setups. Undoing that one small change to get builds green again. Refs: nodejs#7547 (comment) PR-URL: nodejs#7873

Trott · 2016-07-25T19:01:59Z

Proposed quick-fix: #7873

85af1a6 was added (squashed into another commit) without running through CI. Unfortunately, it breaks some of the CI builds, notably on three of the CentOS setups. Undoing that one small change to get builds green again. Refs: nodejs#7547 (comment) PR-URL: nodejs#7873 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

evanlucas · 2016-08-02T12:53:41Z

Is this ABI compatible with v6.x? If so, should it be backported?

addaleax · 2016-08-02T12:56:18Z

@evanlucas It does not touch any public interfaces afaict, so that shouldn’t be a problem.

evanlucas · 2016-08-02T12:57:14Z

Thanks @addaleax!

One of the issues in nodejs#4641 concerns OnConnection in pipe_wrap and tcp_wrap which are very similar with some minor difference in how they are coded. This commit extracts OnConnection so both these classes can share the same implementation. PR-URL: nodejs#7547 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

One of the issues in #4641 concerns OnConnection in pipe_wrap and tcp_wrap which are very similar with some minor difference in how they are coded. This commit extracts OnConnection so both these classes can share the same implementation. PR-URL: #7547 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

85af1a6 was added (squashed into another commit) without running through CI. Unfortunately, it breaks some of the CI builds, notably on three of the CentOS setups. Undoing that one small change to get builds green again. Refs: #7547 (comment) PR-URL: #7873 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. build Issues and PRs related to build files or the CI. labels Jul 5, 2016

mscdex added net Issues and PRs related to the net subsystem. and removed build Issues and PRs related to build files or the CI. labels Jul 5, 2016

addaleax reviewed Jul 7, 2016
View reviewed changes

danbev added 2 commits July 7, 2016 20:21

Moving template types to the ConnectionWrap class.

a3b976e

addaleax reviewed Jul 8, 2016
View reviewed changes

trevnorris reviewed Jul 8, 2016
View reviewed changes

bnoordhuis reviewed Jul 9, 2016
View reviewed changes

danbev added 7 commits July 9, 2016 13:24

Renaming BaseObject's handle_ to objecthandle_

564342d

Changing uvhandle_ back to handle_

623ad2e

Removing unnecessary friend class ConnectionWrap

458accc

Making UVHandle() an inline member function.

ee0f2b9

Adding a space after template keyword

5562c45

Correcting comment changing Instanciate to Instantiate

ee62109

Adding a null pointer check for wrap_data.

579f48a

addaleax reviewed Jul 9, 2016
View reviewed changes

addaleax closed this Jul 25, 2016

addaleax added the dont-land-on-v4.x label Aug 2, 2016

cjihrig mentioned this pull request Aug 8, 2016

v6.4.0 Release Proposal #8020

Closed

cjihrig mentioned this pull request Aug 11, 2016

v6.4.0 Proposal #8070

Merged

danbev deleted the extracting-onconnection branch September 7, 2016 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: extracting OnConnection from pipe_wrap and tcp_wrap #7547

src: extracting OnConnection from pipe_wrap and tcp_wrap #7547

danbev commented Jul 5, 2016

Fishrock123 commented Jul 7, 2016

Fishrock123 commented Jul 7, 2016

addaleax Jul 7, 2016

danbev Jul 7, 2016

addaleax Jul 8, 2016

danbev Jul 8, 2016

addaleax commented Jul 8, 2016

danbev commented Jul 8, 2016

addaleax commented Jul 8, 2016

danbev commented Jul 8, 2016

trevnorris Jul 8, 2016

danbev Jul 9, 2016

trevnorris commented Jul 8, 2016

bnoordhuis Jul 9, 2016

danbev Jul 9, 2016

addaleax Jul 9, 2016

danbev Jul 9, 2016

addaleax commented Jul 25, 2016

jbergstroem commented Jul 25, 2016

Trott commented Jul 25, 2016

Trott commented Jul 25, 2016

evanlucas commented Aug 2, 2016

addaleax commented Aug 2, 2016

evanlucas commented Aug 2, 2016

		using v8::Value;


		template<typename WrapType, typename UVType>


		template uv_pipe_t* ConnectionWrap<PipeWrap, uv_pipe_t>::UVHandle();

		template uv_tcp_t* ConnectionWrap<TCPWrap, uv_tcp_t>::UVHandle();

src: extracting OnConnection from pipe_wrap and tcp_wrap #7547

src: extracting OnConnection from pipe_wrap and tcp_wrap #7547

Conversation

danbev commented Jul 5, 2016

Checklist

Affected core subsystem(s)

Description of change

Fishrock123 commented Jul 7, 2016

Fishrock123 commented Jul 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jul 8, 2016

danbev commented Jul 8, 2016

addaleax commented Jul 8, 2016

danbev commented Jul 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented Jul 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jul 25, 2016

jbergstroem commented Jul 25, 2016

Trott commented Jul 25, 2016

Trott commented Jul 25, 2016

evanlucas commented Aug 2, 2016

addaleax commented Aug 2, 2016

evanlucas commented Aug 2, 2016