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

addaleax · 2017-09-27T20:43:00Z

Also split out from #58

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

Previously, handles would not be closed when the current `Environment` stopped, which is acceptable in a single-`Environment`-per-process situation, but would otherwise create memory and file descriptor leaks.

This allows easier tracking of whether there are active `ReqWrap`s.

Qard · 2017-09-27T21:18:43Z

src/req-wrap-inl.h

+      fn,
+      env()->event_loop(),
+      req(),
+      MakeLibuvRequestCallback<T, Args>::For(this, args)...);


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

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

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. 🙀

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.

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.

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.

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. 😸

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

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.

Workers cannot shut down while requests are open, so keep a counter that is increased whenever libuv requests are made and decreased whenever their callback is called.

addaleax · 2017-09-29T00:33:47Z

MacOS CI should be good now :)

Previously, handles would not be closed when the current `Environment` stopped, which is acceptable in a single-`Environment`-per-process situation, but would otherwise create memory and file descriptor leaks. PR-URL: #85 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

This allows easier tracking of whether there are active `ReqWrap`s. PR-URL: #85 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

Workers cannot shut down while requests are open, so keep a counter that is increased whenever libuv requests are made and decreased whenever their callback is called. PR-URL: #85 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

addaleax · 2017-09-30T12:58:43Z

Landed in 1f66e8b...c4663f3 :)

Original PR: ayojs/ayo#85 > Previously, handles would not be closed when the current `Environment` > stopped, which is acceptable in a single-`Environment`-per-process > situation, but would otherwise create memory and file descriptor > leaks. > PR-URL: ayojs/ayo#85 > Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

Original PR: ayojs/ayo#85 > This allows easier tracking of whether there are active `ReqWrap`s. > PR-URL: ayojs/ayo#85 > Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

Original PR: ayojs/ayo#85 > Workers cannot shut down while requests are open, so keep a counter > that is increased whenever libuv requests are made and decreased > whenever their callback is called. > PR-URL: ayojs/ayo#85 > Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

Previously, handles would not be closed when the current `Environment` stopped, which is acceptable in a single-`Environment`-per-process situation, but would otherwise create memory and file descriptor leaks. Also, introduce a generic way to close handles via the `Environment::CloseHandle()` function, which automatically keeps track of whether a close callback has been called yet or not. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Refs: ayojs/ayo#85 PR-URL: #19377 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

This allows easier tracking of whether there are active `ReqWrap`s. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Refs: ayojs/ayo#85 PR-URL: #19377 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Workers cannot shut down while requests are open, so keep a counter that is increased whenever libuv requests are made and decreased whenever their callback is called. This also applies to other embedders, who may want to shut down an `Environment` instance early. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Fixes: #20517 Refs: ayojs/ayo#85 PR-URL: #19377 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added 2 commits September 27, 2017 22:41

src: make CleanupHandles() tear down handles/reqs

019930c

Previously, handles would not be closed when the current `Environment` stopped, which is acceptable in a single-`Environment`-per-process situation, but would otherwise create memory and file descriptor leaks.

src: unify ReqWrap libuv calling

95839a0

This allows easier tracking of whether there are active `ReqWrap`s.

addaleax added lib / src libuv labels Sep 27, 2017

addaleax requested a review from a team September 27, 2017 20:43

Qard reviewed Sep 27, 2017

View reviewed changes

addaleax added 2 commits September 28, 2017 19:15

[squash] add explanatory comment

55356eb

src: keep track of open requests

a42c922

Workers cannot shut down while requests are open, so keep a counter that is increased whenever libuv requests are made and decreased whenever their callback is called.

addaleax force-pushed the workers-handlecleanup branch from 9b80626 to a42c922 Compare September 28, 2017 17:23

[squash] fix bug

ca1771e

Qard approved these changes Sep 29, 2017

View reviewed changes

addaleax added a commit that referenced this pull request Sep 30, 2017

src: unify ReqWrap libuv calling

c94cd24

This allows easier tracking of whether there are active `ReqWrap`s. PR-URL: #85 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

addaleax closed this Sep 30, 2017

addaleax deleted the workers-handlecleanup branch September 30, 2017 12:58

jasnell mentioned this pull request Oct 1, 2017

src: cherry-pick from downstream, worker prep nodejs/node#15707

Closed

2 tasks

addaleax added the worker label Oct 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

addaleax commented Sep 27, 2017

Qard Sep 27, 2017

addaleax Sep 27, 2017

Qard Sep 27, 2017

Fishrock123 Sep 28, 2017

Qard Sep 28, 2017

addaleax Sep 28, 2017

Qard Sep 28, 2017

addaleax Sep 28, 2017

Qard Sep 28, 2017

addaleax commented Sep 29, 2017

addaleax commented Sep 30, 2017

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

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

Conversation

addaleax commented Sep 27, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Sep 29, 2017

addaleax commented Sep 30, 2017