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: extracting OnConnection from pipe_wrap and tcp_wrap #7547

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
ad6dee3
src: extracting OnConnection from pipe_wrap and tcp_wrap
danbev Jul 4, 2016
a3b976e
Moving template types to the ConnectionWrap class.
danbev Jul 7, 2016
34d5abc
extracting UVHandle() and handle_ to ConnectionWrap.
danbev Jul 8, 2016
564342d
Renaming BaseObject's handle_ to objecthandle_
danbev Jul 9, 2016
623ad2e
Changing uvhandle_ back to handle_
danbev Jul 9, 2016
458accc
Removing unnecessary friend class ConnectionWrap
danbev Jul 9, 2016
ee0f2b9
Making UVHandle() an inline member function.
danbev Jul 9, 2016
5562c45
Adding a space after template keyword
danbev Jul 9, 2016
ee62109
Correcting comment changing Instanciate to Instantiate
danbev Jul 9, 2016
579f48a
Adding a null pointer check for wrap_data.
danbev Jul 9, 2016
1437ec6
Adding back spaces to line up macro continuation escape charaters
danbev Jul 9, 2016
d0da4bc
Removing unnecessary template specializations
danbev Jul 9, 2016
1b3be60
Fixing typo in comment.
danbev Jul 9, 2016
e364ec4
Lining up arguments for SetWeak method call
danbev Jul 14, 2016
e8ed621
Renaming objecthandle_ to persistent_handle_
danbev Jul 14, 2016
7ce112a
Adding a comment to clarify uv_accept error handling
danbev Jul 14, 2016
c9ec24d
Correcting spelling of unavailable.
danbev Jul 14, 2016
5ef85f9
Removing multiple inheritance.
danbev Jul 14, 2016
ece2d31
Making ConnectionWrap inherit from StreamWrap
danbev Jul 16, 2016
73c956d
Adding a destructor for ConnectionWrap
danbev Jul 17, 2016
4feb84a
Removing unnecessary template parameters for OnConnection
danbev Jul 17, 2016
0a1ff38
Removing the inclusion of async-wrap.h
danbev Jul 17, 2016
85af1a6
Removing the empty destructor in favour of an explicit default
danbev Jul 20, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
14 changes: 7 additions & 7 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),
: objecthandle_(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(objecthandle_.IsEmpty());
}


inline v8::Persistent<v8::Object>& BaseObject::persistent() {
return handle_;
return objecthandle_;
}


inline v8::Local<v8::Object> BaseObject::object() {
return PersistentToLocal(env_->isolate(), handle_);
return PersistentToLocal(env_->isolate(), objecthandle_);
}


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>,
objecthandle_.MarkIndependent();
objecthandle_.SetWeak<Type>(ptr, WeakCallback<Type>,
v8::WeakCallbackType::kParameter);
Copy link
Member

Choose a reason for hiding this comment

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

Can you line up the arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, I'll fix this.

}


inline void BaseObject::ClearWeak() {
handle_.ClearWeak();
objecthandle_.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> objecthandle_;
Copy link
Member

Choose a reason for hiding this comment

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

It would be infinitesimally more idiomatic to call it object_handle_ or persistent_handle_ or just object_. Not a blocker, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, let me change that to one of your suggestions.

Environment* env_;
};

Expand Down
69 changes: 69 additions & 0 deletions src/connection_wrap.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
#include "connection_wrap.h"

#include "env-inl.h"
#include "env.h"
#include "pipe_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>
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 as already called
Copy link
Member

Choose a reason for hiding this comment

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

While you’re at it, I realize it’s copypasted but there’s a typo here: if someone has already called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed that, I'll correct it.

// 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_);
if (uv_accept(handle, client_handle))
Copy link
Member

Choose a reason for hiding this comment

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

While you're here, can you add a comment explaining that uv_accept() can fail when the new connection is already closed again by the peer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, I'll add a comment.

return;

// Successful accept. Call the onconnection callback in JavaScript land.
argv[1] = client_obj;
}
wrap_data->MakeCallback(env->onconnection_string(), arraysize(argv), argv);
}

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

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

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

Choose a reason for hiding this comment

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

I think these two can be dropped now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I'll remove them.



} // namespace node
28 changes: 28 additions & 0 deletions src/connection_wrap.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#ifndef SRC_CONNECTION_WRAP_H_
#define SRC_CONNECTION_WRAP_H_

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include "env.h"
#include "v8.h"

namespace node {

template <typename WrapType, typename UVType>
class ConnectionWrap {
public:
UVType* UVHandle() {
return &handle_;
}

protected:
static void OnConnection(uv_stream_t* handle, int status);
UVType handle_;
};


} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#endif // SRC_CONNECTION_WRAP_H_
45 changes: 1 addition & 44 deletions src/pipe_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "req-wrap.h"
#include "req-wrap-inl.h"
#include "stream_wrap.h"
#include "connection_wrap.h"
#include "util-inl.h"
#include "util.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 @@ -167,44 +162,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
8 changes: 2 additions & 6 deletions src/pipe_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,12 @@
#include "async-wrap.h"
#include "env.h"
#include "stream_wrap.h"
#include "connection_wrap.h"

namespace node {

class PipeWrap : public StreamWrap {
class PipeWrap : public StreamWrap, 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 +36,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
44 changes: 1 addition & 43 deletions src/tcp_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "req-wrap.h"
#include "req-wrap-inl.h"
#include "stream_wrap.h"
#include "connection_wrap.h"
#include "util.h"
#include "util-inl.h"

Expand All @@ -28,7 +29,6 @@ using v8::Integer;
using v8::Local;
using v8::Object;
using v8::String;
using v8::Undefined;
using v8::Value;


Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -258,43 +253,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);
Expand Down
8 changes: 2 additions & 6 deletions src/tcp_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,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 StreamWrap, 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:
Expand Down Expand Up @@ -45,10 +44,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_;
};


Expand Down
2 changes: 1 addition & 1 deletion src/udp_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ void UDPWrap::Bind6(const FunctionCallbackInfo<Value>& args) {
UDPWrap* wrap = Unwrap<UDPWrap>(args.Holder()); \
CHECK_EQ(args.Length(), 1); \
int flag = args[0]->Int32Value(); \
int err = wrap == nullptr ? UV_EBADF : fn(&wrap->handle_, flag); \
int err = wrap == nullptr ? UV_EBADF : fn(&wrap->handle_, flag); \
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind re-adding the spaces here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that. I'll add them back.

args.GetReturnValue().Set(err); \
}

Expand Down