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

jasnell · 2017-10-01T06:14:32Z

Cherry-pick of downstream ayo commits.

Checklist

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

Affected core subsystem(s)

src, async_wrap, cares

Original PR: ayojs/ayo#82 > This makes some of the internal `AsyncWrap::MakeCallback()` > utility wrappers throw rather than crash the process when > the requested method is not present on the `this` object. > Doing so makes it easier for future code to expose C++ > objects directly to userland, where JS code can overwrite > or delete such methods. > PR-URL: ayojs/ayo#82 > Reviewed-By: Stephen Belanger <[email protected]>

Original PR: ayojs/ayo#82 > Previously, the `Environment` destructor did not release its > `fs_stats_field_array` memory. The memory is allocated when the > `fs` module is initialized and calls `GetStatValues()`. > PR-URL: ayojs/ayo#82 > Reviewed-By: Stephen Belanger <[email protected]>

Original PR: ayojs/ayo#82 > PR-URL: ayojs/ayo#82 > Reviewed-By: Stephen Belanger <[email protected]>

Original PR: ayojs/ayo#82 > This adds pairs of methods to the `Environment` class and to public APIs > which can add and remove cleanup handlers. > Unlike `AtExit`, this API targets addon developers rather than > embedders, giving them (and Node’s internals) the ability to register > per-`Environment` cleanup work. > PR-URL: ayojs/ayo#82 > Reviewed-By: Stephen Belanger <[email protected]>

Original PR: ayojs/ayo#82 > PR-URL: ayojs/ayo#82 > Reviewed-By: Stephen Belanger <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

mscdex · 2017-10-01T06:49:32Z

Why were the changes to string_search.{cc,h} made in 53d6465? Shouldn't that have been a separate commit?

addaleax · 2017-10-01T09:39:33Z

@jasnell I don’t think I can approve these changes as the author :)

jasnell · 2017-10-02T16:53:33Z

@mscdex

Why were the changes to string_search ... Shouldn't that have been a separate commit?

Likely, but I didn't make those changes so I cannot say for certain

@addaleax

I don't think I can approve these changes as the author

No, but you can, if you'd like, review them for completeness :-) If you wouldn't mind.

jasnell · 2017-10-02T20:01:01Z

Ping @nodejs/collaborators

jasnell · 2017-10-02T20:01:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10383/

mcollina · 2017-10-02T20:22:12Z

How are we copyright-wise for this?

jkrems · 2017-10-02T20:35:58Z

Assuming this counts as a derived work of the original code and since it shares the same license headers, I wouldn't expect this to be a problem in terms of copyright..? This is copying public code under MIT w/o changing the license.

P.S.: If Anna would prefer not pulling this in, we should respect that of course. But that's goes beyond copyright. :)

jasnell · 2017-10-02T21:47:00Z

There should be absolutely zero copyright issue here at all.

refack · 2017-10-02T22:32:51Z

Can we have a short post implementation eps? What's the motivation? What the key implementation principles?

I saw some Mutexes in the code. If the Ayo worker implementation is based on threads I'm -1 without discussing the alternatives, or much more field testing.

refack

Introducing Mutexes and threads without any userland implementation, test coverage or correctness proof.

I know this is an "obscure" CR, but it's also an abstract change set.

Qard · 2017-10-02T22:42:38Z

It's just Mutex::ScopedLock on sync code. Nothing to worry about.

Also, this is just preliminary work. There's no implementation yet because it needs these bits to even be possible. Test coverage will be included with the implementation, when it's ready.

refack · 2017-10-02T22:47:15Z

It's just Mutex::ScopedLock on sync code. Nothing to worry about.

In that case I'm not sure what is the benefit?
If this is in preparation for pulling a big batch of thread work, then IMHO it should be reviewed in context.
If this solves bugs IMHO it should come with regression tests.

Qard · 2017-10-02T23:05:42Z

If it actually got reviewed that'd be fine, but the previous workers PR was left for literally years without proper review.

It's a really big feature. Delivering it all in one neat package is a huge undertaking. It needs to happen in stages or it'll just never happen.

trevnorris · 2017-10-02T23:07:13Z

@addaleax I'm cool with the changes to async-wrap-inl.h.

@jasnell Some of this (e.g. the "dark template magic" or the implementation of Environment::CloseHandle()) should be discussed in more detail.

Qard · 2017-10-02T23:16:43Z

For reference, the worker PR itself is ayojs/ayo#58. These commits were picked out of that to separate PRs because it was difficult to review such a large block of code all at once.

refack

I believe some of this should move to libuv

refack · 2017-10-02T23:50:49Z

src/env-inl.h

+  });
+}
+
+void Environment::IncreaseWaitingRequestCounter() {


AFAICT this is supposed to proxy the status of the libuv. If I'm correct IMHO it should be a libuv API.
libuv/libuv#1528 seems like a good start.

refack · 2017-10-02T23:58:01Z

Diving in and starting to figure out some bits. It seems to me this PR as is, does benefit the use-case of embedding node into an MT process, but I'm deducing this from the code. IMHO it would help if it came with some explanations and motivation.

addaleax · 2017-10-03T00:35:50Z

@refack Yes, maybe that should have ended up in commit messages

addaleax · 2017-10-03T00:39:23Z

Oops, pressed enter too early. Anyway, maybe the commit messages for adding mutexesshould have provided motivation, but I think you can basically deduce from “add mutexes to previously single-threaded code” what you did deduce from it.

Also, yes, I’m fine copyright-wise.

And re: completeness … I see nothing missing here, but since the work in Ayo.js is not complete, it’s hard to make a statement on that.

bnoordhuis · 2017-10-03T09:51:37Z

I still think the single-process approach is a fundamentally bad idea for reasons of robustness and portability, see #13143 (comment) for background.

The multi-process approach on the other hand is something I could get behind.

Figured out the motivation

refack · 2017-10-04T20:37:15Z

I can get behind this change if we #ifdef the Mutexes to be enabled for the embedding scenario, maybe even just for embedding-in-MT-host scenario.

I'll try to do that bit (while I'm not at #Interactive :( )

benjamingr

These changes generally LGTM and I ran the code (in Ayo) and it worked fine. I'm not sure I'm the right person to weigh in on the code but the code changes themselves lgtm

benjamingr · 2017-10-05T23:36:10Z

src/req-wrap-inl.h

+  uv_cancel(reinterpret_cast<uv_req_t*>(&req_));
+}
+
+/* Below is dark template magic designed to invoke libuv functions that


This is fairly standard C++, I'm not sure why it consists of magic but I like the explanation. I'm a little surprised this works on all Node's compile targets though.

benjamingr · 2017-10-05T23:46:21Z

src/req-wrap-inl.h

+      req(),
+      MakeLibuvRequestCallback<T, Args>::For(this, args)...);
+  if (err >= 0)
+    env()->IncreaseWaitingRequestCounter();


I think it's worth commenting that err < 0 always implies that the callback won't be called, so we don't decrease/increase the waiting request counter.

BridgeAR · 2017-11-22T18:37:56Z

This needs a rebase due to many conflicts and I think there was some further progress in ayo about this that should also be included in the update.

jasnell · 2017-11-22T18:41:56Z

I'm going to close this for now and will revisit again later on.

addaleax added 9 commits September 30, 2017 22:52

src: protect global state with mutexes

53d6465

Original PR: ayojs/ayo#82 > PR-URL: ayojs/ayo#82 > Reviewed-By: Stephen Belanger <[email protected]>

src: add can_call_into_js flag

ab573ae

Original PR: ayojs/ayo#82 > PR-URL: ayojs/ayo#82 > Reviewed-By: Stephen Belanger <[email protected]>

src: use lock for c-ares library init/cleanup

91848f1

Original PR: ayojs/ayo#82 > PR-URL: ayojs/ayo#82 > Reviewed-By: Stephen Belanger <[email protected]>

src: unify ReqWrap libuv calling

78f2238

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 <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 1, 2017

refack suggested changes Oct 2, 2017

View reviewed changes

refack previously requested changes Oct 2, 2017

View reviewed changes

refack added the embedding Issues and PRs related to embedding Node.js in another project. label Oct 2, 2017

benjamingr approved these changes Oct 5, 2017

View reviewed changes

jasnell closed this Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jasnell commented Oct 1, 2017

mscdex commented Oct 1, 2017

addaleax commented Oct 1, 2017

jasnell commented Oct 2, 2017

jasnell commented Oct 2, 2017

jasnell commented Oct 2, 2017

mcollina commented Oct 2, 2017

jkrems commented Oct 2, 2017 •

edited

Loading

jasnell commented Oct 2, 2017

refack commented Oct 2, 2017

refack left a comment

Qard commented Oct 2, 2017 •

edited

Loading

refack commented Oct 2, 2017

Qard commented Oct 2, 2017

trevnorris commented Oct 2, 2017

Qard commented Oct 2, 2017

refack left a comment

refack Oct 2, 2017

refack commented Oct 2, 2017

addaleax commented Oct 3, 2017

addaleax commented Oct 3, 2017

bnoordhuis commented Oct 3, 2017

refack commented Oct 4, 2017

benjamingr left a comment

benjamingr Oct 5, 2017

benjamingr Oct 5, 2017

BridgeAR commented Nov 22, 2017

jasnell commented Nov 22, 2017

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

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

Conversation

jasnell commented Oct 1, 2017

Checklist

Affected core subsystem(s)

mscdex commented Oct 1, 2017

addaleax commented Oct 1, 2017

jasnell commented Oct 2, 2017

jasnell commented Oct 2, 2017

jasnell commented Oct 2, 2017

mcollina commented Oct 2, 2017

jkrems commented Oct 2, 2017 • edited Loading

jasnell commented Oct 2, 2017

refack commented Oct 2, 2017

refack left a comment

Choose a reason for hiding this comment

Qard commented Oct 2, 2017 • edited Loading

refack commented Oct 2, 2017

Qard commented Oct 2, 2017

trevnorris commented Oct 2, 2017

Qard commented Oct 2, 2017

refack left a comment

Choose a reason for hiding this comment

refack Oct 2, 2017

Choose a reason for hiding this comment

refack commented Oct 2, 2017

addaleax commented Oct 3, 2017

addaleax commented Oct 3, 2017

bnoordhuis commented Oct 3, 2017

refack commented Oct 4, 2017

benjamingr left a comment

Choose a reason for hiding this comment

benjamingr Oct 5, 2017

Choose a reason for hiding this comment

benjamingr Oct 5, 2017

Choose a reason for hiding this comment

BridgeAR commented Nov 22, 2017

jasnell commented Nov 22, 2017

jkrems commented Oct 2, 2017 •

edited

Loading

Qard commented Oct 2, 2017 •

edited

Loading