Skip to content

Commit

Permalink
src: pull OnConnection from pipe_wrap and tcp_wrap
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
danbev authored and addaleax committed Jul 25, 2016
1 parent b3127df commit 4663393
Show file tree
Hide file tree
Showing 9 changed files with 154 additions and 120 deletions.
2 changes: 2 additions & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@
'src/env.cc',
'src/fs_event_wrap.cc',
'src/cares_wrap.cc',
'src/connection_wrap.cc',
'src/handle_wrap.cc',
'src/js_stream.cc',
'src/node.cc',
Expand Down Expand Up @@ -177,6 +178,7 @@
'src/async-wrap-inl.h',
'src/base-object.h',
'src/base-object-inl.h',
'src/connection_wrap.h',
'src/debug-agent.h',
'src/env.h',
'src/env-inl.h',
Expand Down
16 changes: 8 additions & 8 deletions src/base-object-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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_);
}


Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/base-object.h
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
};

Expand Down
93 changes: 93 additions & 0 deletions src/connection_wrap.cc
Original file line number Diff line number Diff line change
@@ -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
36 changes: 36 additions & 0 deletions src/connection_wrap.h
Original file line number Diff line number Diff line change
@@ -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_
54 changes: 5 additions & 49 deletions src/pipe_wrap.cc
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -27,7 +28,6 @@ using v8::Integer;
using v8::Local;
using v8::Object;
using v8::String;
using v8::Undefined;
using v8::Value;


Expand All @@ -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());
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down
9 changes: 2 additions & 7 deletions src/pipe_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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_;
};


Expand Down
Loading

0 comments on commit 4663393

Please sign in to comment.