Skip to content
This repository has been archived by the owner on Aug 31, 2018. It is now read-only.

src: clean up handles (preparation for workers) #85

Closed
wants to merge 5 commits into from
Closed

src: clean up handles (preparation for workers) #85

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
019930c
src: make CleanupHandles() tear down handles/reqs
addaleax Sep 4, 2017
95839a0
src: unify ReqWrap libuv calling
addaleax Sep 25, 2017
55356eb
[squash] add explanatory comment
addaleax Sep 28, 2017
a42c922
src: keep track of open requests
addaleax Sep 25, 2017
ca1771e
[squash] fix bug
addaleax Sep 29, 2017
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
22 changes: 9 additions & 13 deletions src/cares_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1969,13 +1969,11 @@ void GetAddrInfo(const FunctionCallbackInfo<Value>& args) {
hints.ai_socktype = SOCK_STREAM;
hints.ai_flags = flags;

int err = uv_getaddrinfo(env->event_loop(),
req_wrap->req(),
AfterGetAddrInfo,
*hostname,
nullptr,
&hints);
req_wrap->Dispatched();
int err = req_wrap->Dispatch(uv_getaddrinfo,
AfterGetAddrInfo,
*hostname,
nullptr,
&hints);
if (err)
delete req_wrap;

Expand All @@ -1999,12 +1997,10 @@ void GetNameInfo(const FunctionCallbackInfo<Value>& args) {

GetNameInfoReqWrap* req_wrap = new GetNameInfoReqWrap(env, req_wrap_obj);

int err = uv_getnameinfo(env->event_loop(),
req_wrap->req(),
AfterGetNameInfo,
(struct sockaddr*)&addr,
NI_NAMEREQD);
req_wrap->Dispatched();
int err = req_wrap->Dispatch(uv_getnameinfo,
AfterGetNameInfo,
(struct sockaddr*)&addr,
NI_NAMEREQD);
if (err)
delete req_wrap;

Expand Down
19 changes: 7 additions & 12 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,11 +355,7 @@ class fs_req_wrap {
CHECK(request->IsObject()); \
FSReqWrap* req_wrap = FSReqWrap::New(env, request.As<Object>(), \
#func, dest, encoding); \
int err = uv_fs_ ## func(env->event_loop(), \
req_wrap->req(), \
__VA_ARGS__, \
After); \
req_wrap->Dispatched(); \
int err = req_wrap->Dispatch(uv_fs_ ## func, __VA_ARGS__, After); \
if (err < 0) { \
uv_fs_t* uv_req = req_wrap->req(); \
uv_req->result = err; \
Expand Down Expand Up @@ -1123,13 +1119,12 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {

FSReqWrap* req_wrap =
FSReqWrap::New(env, req.As<Object>(), "write", buf, UTF8, ownership);
int err = uv_fs_write(env->event_loop(),
req_wrap->req(),
fd,
&uvbuf,
1,
pos,
After);
int err = req_wrap->Dispatch(uv_fs_write,
fd,
&uvbuf,
1,
pos,
After);
req_wrap->Dispatched();
if (err < 0) {
uv_fs_t* uv_req = req_wrap->req();
Expand Down
9 changes: 4 additions & 5 deletions src/pipe_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,10 @@ void PipeWrap::Connect(const FunctionCallbackInfo<Value>& args) {

ConnectWrap* req_wrap =
new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_PIPECONNECTWRAP);
uv_pipe_connect(req_wrap->req(),
&wrap->handle_,
*name,
AfterConnect);
req_wrap->Dispatched();
req_wrap->Dispatch(uv_pipe_connect,
&wrap->handle_,
*name,
AfterConnect);

args.GetReturnValue().Set(0); // uv_pipe_connect() doesn't return errors.
}
Expand Down
89 changes: 89 additions & 0 deletions src/req-wrap-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,95 @@ void ReqWrap<T>::Cancel() {
uv_cancel(reinterpret_cast<uv_req_t*>(&req_));
}

/* Below is dark template magic designed to invoke libuv functions that
* initialize uv_req_t instances in a unified fashion, to allow easier
* tracking of active/inactive requests. */

// Invoke a generic libuv function that initializes uv_req_t instances.
// This is, unfortunately, necessary since they come in three different
// variants that can not all be invoked in the same way:
// - int uv_foo(uv_loop_t* loop, uv_req_t* request, ...);
// - int uv_foo(uv_req_t* request, ...);
// - void uv_foo(uv_req_t* request, ...);
template<typename ReqT, typename T>
struct CallLibuvFunction;

// Detect `int uv_foo(uv_loop_t* loop, uv_req_t* request, ...);`.
template<typename ReqT, typename... Args>
struct CallLibuvFunction<ReqT, int(*)(uv_loop_t*, ReqT*, Args...)> {
typedef int(*T)(uv_loop_t*, ReqT*, Args...);
template<typename... PassedArgs>
static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) {
return fn(loop, req, args...);
}
};

// Detect `int uv_foo(uv_req_t* request, ...);`.
template<typename ReqT, typename... Args>
struct CallLibuvFunction<ReqT, int(*)(ReqT*, Args...)> {
typedef int(*T)(ReqT*, Args...);
template<typename... PassedArgs>
static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) {
return fn(req, args...);
}
};

// Detect `void uv_foo(uv_req_t* request, ...);`.
template<typename ReqT, typename... Args>
struct CallLibuvFunction<ReqT, void(*)(ReqT*, Args...)> {
typedef void(*T)(ReqT*, Args...);
template<typename... PassedArgs>
static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) {
fn(req, args...);
return 0;
}
};

// This is slightly darker magic: This template is 'applied' to each parameter
// passed to the libuv function. If the parameter type (aka `T`) is a
// function type, it is assumed that this it is the request callback, and a
// wrapper that calls the original callback is created.
// If not, the parameter is passed through verbatim.
template<typename ReqT, typename T>
struct MakeLibuvRequestCallback {
static T For(ReqWrap<ReqT>* req_wrap, T v) {
static_assert(!std::is_function<T>::value,
"MakeLibuvRequestCallback missed a callback");
return v;
}
};

// Match the `void callback(uv_req_t*, ...);` signature that all libuv
// callbacks use.
template<typename ReqT, typename... Args>
struct MakeLibuvRequestCallback<ReqT, void(*)(ReqT*, Args...)> {
typedef void(*T)(ReqT* req, Args... args);

static void Wrapper(ReqT* req, Args... args) {
ReqWrap<ReqT>* req_wrap = ContainerOf(&ReqWrap<ReqT>::req_, req);
T original_callback = reinterpret_cast<T>(req_wrap->original_callback_);
original_callback(req, args...);
}

static T For(ReqWrap<ReqT>* req_wrap, T v) {
CHECK_EQ(req_wrap->original_callback_, nullptr);
req_wrap->original_callback_ =
reinterpret_cast<typename ReqWrap<ReqT>::callback_t>(v);
return Wrapper;
}
};

template <typename T>
template <typename LibuvFunction, typename... Args>
int ReqWrap<T>::Dispatch(LibuvFunction fn, Args... args) {
Dispatched();
return CallLibuvFunction<T, LibuvFunction>::Call(
fn,
env()->event_loop(),
req(),
MakeLibuvRequestCallback<T, Args>::For(this, args)...);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm misreading, but doesn't MakeLibuvRequestCallback<...>::For(...) make the callback variadic, not the call? For is just returning a single T function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sooo … I am not sure I’m getting the question right 😄 What this does is write MakeLibuvRequestCallback<T, Args>::For(this, ?) for every arg ? in args.

http://en.cppreference.com/w/cpp/language/parameter_pack#Pack_expansion can explain this better than I can :)

Copy link
Member

Choose a reason for hiding this comment

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

Ohhh...I get it now. Thanks for the clarification.

Perhaps all that template wizardry could use some more elaborate comments? I suspect a lot of people less familiar with C++ templates will look at that stuff and have no idea what is going on. 🙀

Copy link
Contributor

Choose a reason for hiding this comment

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

Lots of core could use more comments tbh.

Unfortunately, the template wizardry isn't just ours alone - we simply won't ever fully get away from V8's liberal use of them.

Copy link
Member

Choose a reason for hiding this comment

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

Having a history of poorly documented code doesn't mean we should keep poorly documenting code.

Also, I don't think the V8 comparison quite fits here. The consuming side of the API is fine, it's what is going on behind the scenes that is confusing. Unlike V8, we have to maintain this code entirely ourselves, so it's important we make it understandable to anyone whom may need to work on it in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I’ve added a comment here that should help a bit – @Qard maybe take a look whether this is clear enough :)

One problem here is that this is very subjective – from my perspective what V8 does seems like nothing :) So yes, I really appreciate these comments.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that should be fine. I just want us to get in the habit of making the design of things in core clearer as that will make it a lot more accessible to potential new contributors. There are many people whom could make meaningful contributions to the native code, but don't have a lot of native dev experience or perhaps just haven't worked with C++-specific things like templating. 😸

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 agree 100 % :) It might be nice to write something along the lines of a tutorial for contributing to the native side of things … even people who are used to writing C++ have a bit of a hard time adjusting to the way the V8 API works

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I started working on some Node.js internals docs a few years ago, which I believe are still in the website repo somewhere. I don't think they are exposed in navigation though and could probably be updated and expanded.

}

} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Expand Down
13 changes: 12 additions & 1 deletion src/req-wrap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,25 @@ class ReqWrap : public AsyncWrap {
v8::Local<v8::Object> object,
AsyncWrap::ProviderType provider);
inline ~ReqWrap() override;
inline void Dispatched(); // Call this after the req has been dispatched.
// Call this after the req has been dispatched, if that did not already
// happen by using Dispatch().
inline void Dispatched();
T* req() { return &req_; }
inline void Cancel();

template <typename LibuvFunction, typename... Args>
inline int Dispatch(LibuvFunction fn, Args... args);

private:
friend class Environment;
template<typename ReqT, typename U>
friend struct MakeLibuvRequestCallback;

ListNode<ReqWrap> req_wrap_queue_;

typedef void (*callback_t)();
callback_t original_callback_ = nullptr;

protected:
// req_wrap_queue_ needs to be at a fixed offset from the start of the class
// because it is used by ContainerOf to calculate the address of the embedding
Expand Down
18 changes: 8 additions & 10 deletions src/tcp_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,11 +271,10 @@ void TCPWrap::Connect(const FunctionCallbackInfo<Value>& args) {
env->set_init_trigger_id(wrap->get_id());
ConnectWrap* req_wrap =
new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_TCPCONNECTWRAP);
err = uv_tcp_connect(req_wrap->req(),
&wrap->handle_,
reinterpret_cast<const sockaddr*>(&addr),
AfterConnect);
req_wrap->Dispatched();
err = req_wrap->Dispatch(uv_tcp_connect,
&wrap->handle_,
reinterpret_cast<const sockaddr*>(&addr),
AfterConnect);
if (err)
delete req_wrap;
}
Expand Down Expand Up @@ -307,11 +306,10 @@ void TCPWrap::Connect6(const FunctionCallbackInfo<Value>& args) {
env->set_init_trigger_id(wrap->get_id());
ConnectWrap* req_wrap =
new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_TCPCONNECTWRAP);
err = uv_tcp_connect(req_wrap->req(),
&wrap->handle_,
reinterpret_cast<const sockaddr*>(&addr),
AfterConnect);
req_wrap->Dispatched();
err = req_wrap->Dispatch(uv_tcp_connect,
&wrap->handle_,
reinterpret_cast<const sockaddr*>(&addr),
AfterConnect);
if (err)
delete req_wrap;
}
Expand Down
13 changes: 6 additions & 7 deletions src/udp_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,15 +384,14 @@ void UDPWrap::DoSend(const FunctionCallbackInfo<Value>& args, int family) {
}

if (err == 0) {
err = uv_udp_send(req_wrap->req(),
&wrap->handle_,
*bufs,
count,
reinterpret_cast<const sockaddr*>(&addr),
OnSend);
err = req_wrap->Dispatch(uv_udp_send,
&wrap->handle_,
*bufs,
count,
reinterpret_cast<const sockaddr*>(&addr),
OnSend);
}

req_wrap->Dispatched();
if (err)
delete req_wrap;

Expand Down