Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
64 changes: 64 additions & 0 deletions src/connection_wrap.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
#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>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tiny style nit: space before <.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll correct this.

void ConnectionWrap::OnConnection(uv_stream_t* handle, int status) {
WrapType* wrap_data = static_cast<WrapType*>(handle->data);
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
Copy Markdown
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
Copy Markdown
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) {
// Instanciate the client javascript object and handle.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instantiate? I realize it's an existing comment but since you're here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No problems, I'll correct this.

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
Copy Markdown
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
Copy Markdown
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::OnConnection<PipeWrap, uv_pipe_t>(
uv_stream_t* handle,
int status);

template void ConnectionWrap::OnConnection<TCPWrap, uv_tcp_t>(
uv_stream_t* handle,
int status);


} // namespace node
22 changes: 22 additions & 0 deletions src/connection_wrap.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#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 {

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

static void OnConnection(uv_stream_t* handle, int status);
};


} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#endif // SRC_CONNECTION_WRAP_H_
42 changes: 2 additions & 40 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 Down Expand Up @@ -162,49 +162,11 @@ void PipeWrap::Listen(const FunctionCallbackInfo<Value>& args) {
int backlog = args[0]->Int32Value();
int err = uv_listen(reinterpret_cast<uv_stream_t*>(&wrap->handle_),
backlog,
OnConnection);
ConnectionWrap::OnConnection<PipeWrap, uv_pipe_t>);
args.GetReturnValue().Set(err);
}


// 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
5 changes: 3 additions & 2 deletions src/pipe_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@
#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, ConnectionWrap {
public:
uv_pipe_t* UVHandle();

Expand All @@ -21,6 +22,7 @@ class PipeWrap : public StreamWrap {
size_t self_size() const override { return sizeof(*this); }

private:
friend class ConnectionWrap; // So handle_ can be accessed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

uvhandle_?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm just about to push a few commits, one of which changes the name back to handle_, but also one that removes this friend class as it was no longer required.

PipeWrap(Environment* env,
v8::Local<v8::Object> object,
bool ipc,
Expand All @@ -37,7 +39,6 @@ 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
41 changes: 2 additions & 39 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 @@ -253,48 +253,11 @@ void TCPWrap::Listen(const FunctionCallbackInfo<Value>& args) {
int backlog = args[0]->Int32Value();
int err = uv_listen(reinterpret_cast<uv_stream_t*>(&wrap->handle_),
backlog,
OnConnection);
ConnectionWrap::OnConnection<TCPWrap, uv_tcp_t>);
args.GetReturnValue().Set(err);
}


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
5 changes: 3 additions & 2 deletions src/tcp_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@
#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, ConnectionWrap {
public:
static v8::Local<v8::Object> Instantiate(Environment* env, AsyncWrap* parent);
static void Initialize(v8::Local<v8::Object> target,
Expand All @@ -21,6 +22,7 @@ class TCPWrap : public StreamWrap {
size_t self_size() const override { return sizeof(*this); }

private:
friend class ConnectionWrap; // So handle_ can be accessed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No longer necessary, is it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah good point, I'll remove this.

typedef uv_tcp_t HandleType;

template <typename T,
Expand All @@ -45,7 +47,6 @@ 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