src: pull OnConnection from pipe_wrap and tcp_wrap

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: https://github.com/nodejs/node/pull/7547
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

8 files changed, 152 insertions, 120 deletions

diff --git a/src/base-object-inl.h b/src/base-object-inl.h
index b4c3730f1ae..4ddc5e1349b 100644
--- a/src/base-object-inl.h
+++ b/src/base-object-inl.h
@@ -13,7 +13,7 @@
 namespace node {
 
 inline BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> handle)
-    : handle_(env->isolate(), handle),
+    : persistent_handle_(env->isolate(), handle),
       env_(env) {
   CHECK_EQ(false, handle.IsEmpty());
   // The zero field holds a pointer to the handle. Immediately set it to
@@ -24,17 +24,17 @@ inline BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> handle)
 
 
 inline BaseObject::~BaseObject() {
-  CHECK(handle_.IsEmpty());
+  CHECK(persistent_handle_.IsEmpty());
 }
 
 
 inline v8::Persistent<v8::Object>& BaseObject::persistent() {
-  return handle_;
+  return persistent_handle_;
 }
 
 
 inline v8::Local<v8::Object> BaseObject::object() {
-  return PersistentToLocal(env_->isolate(), handle_);
+  return PersistentToLocal(env_->isolate(), persistent_handle_);
 }
 
 
@@ -58,14 +58,14 @@ inline void BaseObject::MakeWeak(Type* ptr) {
   v8::Local<v8::Object> handle = object();
   CHECK_GT(handle->InternalFieldCount(), 0);
   Wrap(handle, ptr);
-  handle_.MarkIndependent();
-  handle_.SetWeak<Type>(ptr, WeakCallback<Type>,
-                        v8::WeakCallbackType::kParameter);
+  persistent_handle_.MarkIndependent();
+  persistent_handle_.SetWeak<Type>(ptr, WeakCallback<Type>,
+                                   v8::WeakCallbackType::kParameter);
 }
 
 
 inline void BaseObject::ClearWeak() {
-  handle_.ClearWeak();
+  persistent_handle_.ClearWeak();
 }
 
 }  // namespace node
diff --git a/src/base-object.h b/src/base-object.h
index afec3442ce8..27251379770 100644
--- a/src/base-object.h
+++ b/src/base-object.h
@@ -44,7 +44,7 @@ class BaseObject {
   static inline void WeakCallback(
       const v8::WeakCallbackInfo<Type>& data);
 
-  v8::Persistent<v8::Object> handle_;
+  v8::Persistent<v8::Object> persistent_handle_;
   Environment* env_;
 };
 
diff --git a/src/connection_wrap.cc b/src/connection_wrap.cc
new file mode 100644
index 00000000000..f98fb79eb91
--- /dev/null
+++ b/src/connection_wrap.cc
@@ -0,0 +1,93 @@
+#include "connection_wrap.h"
+
+#include "env-inl.h"
+#include "env.h"
+#include "pipe_wrap.h"
+#include "stream_wrap.h"
+#include "tcp_wrap.h"
+#include "util.h"
+#include "util-inl.h"
+
+namespace node {
+
+using v8::Context;
+using v8::HandleScope;
+using v8::Integer;
+using v8::Local;
+using v8::Object;
+using v8::Value;
+
+
+template <typename WrapType, typename UVType>
+ConnectionWrap<WrapType, UVType>::ConnectionWrap(Environment* env,
+                                                 Local<Object> object,
+                                                 ProviderType provider,
+                                                 AsyncWrap* parent)
+    : StreamWrap(env,
+                 object,
+                 reinterpret_cast<uv_stream_t*>(&handle_),
+                 provider,
+                 parent) {}
+
+
+template <typename WrapType, typename UVType>
+void ConnectionWrap<WrapType, UVType>::OnConnection(uv_stream_t* handle,
+                                                    int status) {
+  WrapType* wrap_data = static_cast<WrapType*>(handle->data);
+  CHECK_NE(wrap_data, nullptr);
+  CHECK_EQ(&wrap_data->handle_, reinterpret_cast<UVType*>(handle));
+
+  Environment* env = wrap_data->env();
+  HandleScope handle_scope(env->isolate());
+  Context::Scope context_scope(env->context());
+
+  // We should not be getting this callback if someone has already called
+  // uv_close() on the handle.
+  CHECK_EQ(wrap_data->persistent().IsEmpty(), false);
+
+  Local<Value> argv[] = {
+    Integer::New(env->isolate(), status),
+    Undefined(env->isolate())
+  };
+
+  if (status == 0) {
+    // Instantiate the client javascript object and handle.
+    Local<Object> client_obj = WrapType::Instantiate(env, wrap_data);
+
+    // Unwrap the client javascript object.
+    WrapType* wrap;
+    ASSIGN_OR_RETURN_UNWRAP(&wrap, client_obj);
+    uv_stream_t* client_handle =
+        reinterpret_cast<uv_stream_t*>(&wrap->handle_);
+    // uv_accept can fail if the new connection has already been closed, in
+    // which case an EAGAIN (resource temporarily unavailable) will be
+    // returned.
+    if (uv_accept(handle, client_handle))
+      return;
+
+    // Successful accept. Call the onconnection callback in JavaScript land.
+    argv[1] = client_obj;
+  }
+  wrap_data->MakeCallback(env->onconnection_string(), arraysize(argv), argv);
+}
+
+template ConnectionWrap<PipeWrap, uv_pipe_t>::ConnectionWrap(
+    Environment* env,
+    Local<Object> object,
+    ProviderType provider,
+    AsyncWrap* parent);
+
+template ConnectionWrap<TCPWrap, uv_tcp_t>::ConnectionWrap(
+    Environment* env,
+    Local<Object> object,
+    ProviderType provider,
+    AsyncWrap* parent);
+
+template void ConnectionWrap<PipeWrap, uv_pipe_t>::OnConnection(
+    uv_stream_t* handle, int status);
+
+template void ConnectionWrap<TCPWrap, uv_tcp_t>::OnConnection(
+    uv_stream_t* handle, int status);
+
+
+}  // namespace node
diff --git a/src/connection_wrap.h b/src/connection_wrap.h
new file mode 100644
index 00000000000..7f7a50ae7ca
--- /dev/null
+++ b/src/connection_wrap.h
@@ -0,0 +1,36 @@
+#ifndef SRC_CONNECTION_WRAP_H_
+#define SRC_CONNECTION_WRAP_H_
+
+#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
+
+#include "env.h"
+#include "stream_wrap.h"
+#include "v8.h"
+
+namespace node {
+
+template <typename WrapType, typename UVType>
+class ConnectionWrap : public StreamWrap {
+ public:
+  UVType* UVHandle() {
+    return &handle_;
+  }
+
+  static void OnConnection(uv_stream_t* handle, int status);
+
+ protected:
+  ConnectionWrap(Environment* env,
+                 v8::Local<v8::Object> object,
+                 ProviderType provider,
+                 AsyncWrap* parent);
+  ~ConnectionWrap() = default;
+
+  UVType handle_;
+};
+
+
+}  // namespace node
+
+#endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
+
+#endif  // SRC_CONNECTION_WRAP_H_
diff --git a/src/pipe_wrap.cc b/src/pipe_wrap.cc
index 286ea30a87c..8baf04ba7ce 100644
--- a/src/pipe_wrap.cc
+++ b/src/pipe_wrap.cc
@@ -1,6 +1,7 @@
 #include "pipe_wrap.h"
 
 #include "async-wrap.h"
+#include "connection_wrap.h"
 #include "env.h"
 #include "env-inl.h"
 #include "handle_wrap.h"
@@ -27,7 +28,6 @@ using v8::Integer;
 using v8::Local;
 using v8::Object;
 using v8::String;
-using v8::Undefined;
 using v8::Value;
 
 
@@ -51,11 +51,6 @@ static void NewPipeConnectWrap(const FunctionCallbackInfo<Value>& args) {
 }
 
 
-uv_pipe_t* PipeWrap::UVHandle() {
-  return &handle_;
-}
-
-
 Local<Object> PipeWrap::Instantiate(Environment* env, AsyncWrap* parent) {
   EscapableHandleScope handle_scope(env->isolate());
   CHECK_EQ(false, env->pipe_constructor_template().IsEmpty());
@@ -125,11 +120,10 @@ PipeWrap::PipeWrap(Environment* env,
                    Local<Object> object,
                    bool ipc,
                    AsyncWrap* parent)
-    : StreamWrap(env,
-                 object,
-                 reinterpret_cast<uv_stream_t*>(&handle_),
-                 AsyncWrap::PROVIDER_PIPEWRAP,
-                 parent) {
+    : ConnectionWrap(env,
+                     object,
+                     AsyncWrap::PROVIDER_PIPEWRAP,
+                     parent) {
   int r = uv_pipe_init(env->event_loop(), &handle_, ipc);
   CHECK_EQ(r, 0);  // How do we proxy this error up to javascript?
                    // Suggestion: uv_pipe_init() returns void.
@@ -167,44 +161,6 @@ void PipeWrap::Listen(const FunctionCallbackInfo<Value>& args) {
 }
 
 
-// TODO(bnoordhuis) maybe share with TCPWrap?
-void PipeWrap::OnConnection(uv_stream_t* handle, int status) {
-  PipeWrap* pipe_wrap = static_cast<PipeWrap*>(handle->data);
-  CHECK_EQ(&pipe_wrap->handle_, reinterpret_cast<uv_pipe_t*>(handle));
-
-  Environment* env = pipe_wrap->env();
-  HandleScope handle_scope(env->isolate());
-  Context::Scope context_scope(env->context());
-
-  // We should not be getting this callback if someone as already called
-  // uv_close() on the handle.
-  CHECK_EQ(pipe_wrap->persistent().IsEmpty(), false);
-
-  Local<Value> argv[] = {
-    Integer::New(env->isolate(), status),
-    Undefined(env->isolate())
-  };
-
-  if (status != 0) {
-    pipe_wrap->MakeCallback(env->onconnection_string(), arraysize(argv), argv);
-    return;
-  }
-
-  // Instanciate the client javascript object and handle.
-  Local<Object> client_obj = Instantiate(env, pipe_wrap);
-
-  // Unwrap the client javascript object.
-  PipeWrap* wrap;
-  ASSIGN_OR_RETURN_UNWRAP(&wrap, client_obj);
-  uv_stream_t* client_handle = reinterpret_cast<uv_stream_t*>(&wrap->handle_);
-  if (uv_accept(handle, client_handle))
-    return;
-
-  // Successful accept. Call the onconnection callback in JavaScript land.
-  argv[1] = client_obj;
-  pipe_wrap->MakeCallback(env->onconnection_string(), arraysize(argv), argv);
-}
-
 // TODO(bnoordhuis) Maybe share this with TCPWrap?
 void PipeWrap::AfterConnect(uv_connect_t* req, int status) {
   PipeConnectWrap* req_wrap = static_cast<PipeConnectWrap*>(req->data);
diff --git a/src/pipe_wrap.h b/src/pipe_wrap.h
index f4784ac13a0..1e11221abc5 100644
--- a/src/pipe_wrap.h
+++ b/src/pipe_wrap.h
@@ -4,15 +4,13 @@
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #include "async-wrap.h"
+#include "connection_wrap.h"
 #include "env.h"
-#include "stream_wrap.h"
 
 namespace node {
 
-class PipeWrap : public StreamWrap {
+class PipeWrap : public ConnectionWrap<PipeWrap, uv_pipe_t> {
  public:
-  uv_pipe_t* UVHandle();
-
   static v8::Local<v8::Object> Instantiate(Environment* env, AsyncWrap* parent);
   static void Initialize(v8::Local<v8::Object> target,
                          v8::Local<v8::Value> unused,
@@ -37,10 +35,7 @@ class PipeWrap : public StreamWrap {
       const v8::FunctionCallbackInfo<v8::Value>& args);
 #endif
 
-  static void OnConnection(uv_stream_t* handle, int status);
   static void AfterConnect(uv_connect_t* req, int status);
-
-  uv_pipe_t handle_;
 };
 
 
diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc
index 6904b27efd5..674c0592541 100644
--- a/src/tcp_wrap.cc
+++ b/src/tcp_wrap.cc
@@ -1,5 +1,6 @@
 #include "tcp_wrap.h"
 
+#include "connection_wrap.h"
 #include "env.h"
 #include "env-inl.h"
 #include "handle_wrap.h"
@@ -28,7 +29,6 @@ using v8::Integer;
 using v8::Local;
 using v8::Object;
 using v8::String;
-using v8::Undefined;
 using v8::Value;
 
 
@@ -121,11 +121,6 @@ void TCPWrap::Initialize(Local<Object> target,
 }
 
 
-uv_tcp_t* TCPWrap::UVHandle() {
-  return &handle_;
-}
-
-
 void TCPWrap::New(const FunctionCallbackInfo<Value>& args) {
   // This constructor should not be exposed to public javascript.
   // Therefore we assert that we are not trying to call this as a
@@ -146,11 +141,10 @@ void TCPWrap::New(const FunctionCallbackInfo<Value>& args) {
 
 
 TCPWrap::TCPWrap(Environment* env, Local<Object> object, AsyncWrap* parent)
-    : StreamWrap(env,
-                 object,
-                 reinterpret_cast<uv_stream_t*>(&handle_),
-                 AsyncWrap::PROVIDER_TCPWRAP,
-                 parent) {
+    : ConnectionWrap(env,
+                     object,
+                     AsyncWrap::PROVIDER_TCPWRAP,
+                     parent) {
   int r = uv_tcp_init(env->event_loop(), &handle_);
   CHECK_EQ(r, 0);  // How do we proxy this error up to javascript?
                    // Suggestion: uv_tcp_init() returns void.
@@ -258,43 +252,6 @@ void TCPWrap::Listen(const FunctionCallbackInfo<Value>& args) {
 }
 
 
-void TCPWrap::OnConnection(uv_stream_t* handle, int status) {
-  TCPWrap* tcp_wrap = static_cast<TCPWrap*>(handle->data);
-  CHECK_EQ(&tcp_wrap->handle_, reinterpret_cast<uv_tcp_t*>(handle));
-  Environment* env = tcp_wrap->env();
-
-  HandleScope handle_scope(env->isolate());
-  Context::Scope context_scope(env->context());
-
-  // We should not be getting this callback if someone as already called
-  // uv_close() on the handle.
-  CHECK_EQ(tcp_wrap->persistent().IsEmpty(), false);
-
-  Local<Value> argv[2] = {
-    Integer::New(env->isolate(), status),
-    Undefined(env->isolate())
-  };
-
-  if (status == 0) {
-    // Instantiate the client javascript object and handle.
-    Local<Object> client_obj =
-        Instantiate(env, static_cast<AsyncWrap*>(tcp_wrap));
-
-    // Unwrap the client javascript object.
-    TCPWrap* wrap = Unwrap<TCPWrap>(client_obj);
-    CHECK_NE(wrap, nullptr);
-    uv_stream_t* client_handle = reinterpret_cast<uv_stream_t*>(&wrap->handle_);
-    if (uv_accept(handle, client_handle))
-      return;
-
-    // Successful accept. Call the onconnection callback in JavaScript land.
-    argv[1] = client_obj;
-  }
-
-  tcp_wrap->MakeCallback(env->onconnection_string(), arraysize(argv), argv);
-}
-
-
 void TCPWrap::AfterConnect(uv_connect_t* req, int status) {
   TCPConnectWrap* req_wrap = static_cast<TCPConnectWrap*>(req->data);
   TCPWrap* wrap = static_cast<TCPWrap*>(req->handle->data);
diff --git a/src/tcp_wrap.h b/src/tcp_wrap.h
index af2d08d1ae8..26eb410c430 100644
--- a/src/tcp_wrap.h
+++ b/src/tcp_wrap.h
@@ -5,19 +5,17 @@
 
 #include "async-wrap.h"
 #include "env.h"
-#include "stream_wrap.h"
+#include "connection_wrap.h"
 
 namespace node {
 
-class TCPWrap : public StreamWrap {
+class TCPWrap : public ConnectionWrap<TCPWrap, uv_tcp_t> {
  public:
   static v8::Local<v8::Object> Instantiate(Environment* env, AsyncWrap* parent);
   static void Initialize(v8::Local<v8::Object> target,
                          v8::Local<v8::Value> unused,
                          v8::Local<v8::Context> context);
 
-  uv_tcp_t* UVHandle();
-
   size_t self_size() const override { return sizeof(*this); }
 
  private:
@@ -45,10 +43,7 @@ class TCPWrap : public StreamWrap {
       const v8::FunctionCallbackInfo<v8::Value>& args);
 #endif
 
-  static void OnConnection(uv_stream_t* handle, int status);
   static void AfterConnect(uv_connect_t* req, int status);
-
-  uv_tcp_t handle_;
 };