src: refactor WriteWrap and ShutdownWrap by addaleax · Pull Request #18676 · nodejs/node · GitHub

Encapsulate stream requests more:

WriteWrap and ShutdownWrap classes are now tailored to the
streams on which they are used. In particular, for most streams
these are now plain AsyncWraps and do not carry the overhead
of unused libuv request data.
Provide generic Write() and Shutdown() methods that wrap
around the actual implementations, and make usage of streams
easier, rather than implementing; for example, wrap objects
don’t need to be provided by callers anymore.
Use EmitAfterWrite() and EmitAfterShutdown() handlers to
call the corresponding JS handlers, rather than always trying
to call them. This makes usage of streams by other C++ code
easier and leaner.

Also fix up some tests that were previously not actually testing
asynchronicity when the comments indicated that they would.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

src

Maybe @apapirovski could take a look some time? :)

CI: https://ci.nodejs.org/job/node-test-commit/16096/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1269/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/118/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/120/

@TimothyGu Since you were asking for ideas, this might be a decent medium-sized project?

I've started reviewing but will probably take a good few passes. Looks great so far. 🙏 🎉

I'm probably missing something but why the static_cast?

It’s because *stream_ is a StreamResource instance, but we know that we only ever use a StreamBase for HTTP2.

One could make a case for merging the two classes, given that it’s unclear whether there will ever be StreamResources that are not tied to an AsyncWrap. I took this path in #18334, because it presents a nice separation of concerns: The actual implementation of a data stream vs all the JS stuff surrounding it.

StreamResource is-a StreamBase so Write() is available, isn't it? The static_cast looks superfluous to me.

StreamResource is-a StreamBase

Unfortunately, it’s the reverse situation. Maybe StreamBase is not the best name for this… :/

Anyway, I’ve added a underlying_stream() helper to Http2Session and TLSWrap that makes the cast transparent, so that shouldn’t be an issue for niow.

Fits on one line.

Is this necessary?

@bnoordhuis I don’t think so. It was carried over from the original code.

I’ve removed it for now, the tests seem to pass and we’ll see if it causes any trouble.

This won't work when #18656 lands.

I don't quite understand why auto-reset is disabled for LibuvStreamWrap. Does it need to?

I see the logic in the TLS code. That's essentially about moving over the JS object from one instance to another?

This won't work when #18656 lands.

I think we could remove the entire conditional, and the template parameter once that lands (yay!). I’ll rebase & remove it once that happens to be sure.

I don't quite understand why auto-reset is disabled for LibuvStreamWrap. Does it need to?

Yes, because the libuv streams use inherit from ReqWrap, which currently also checks that the persistent is not empty inside its destructor and resets it afterwards. (The CHECK there would crash).

That's essentially about moving over the JS object from one instance to another?

Yes, exactly.

Suggestion: use a std::unique_ptr<char> with a custom deleter and .release() it in the call to SetAllocatedStorage().

(Or perhaps even better: pass it to WriteWrap so you don't have to free the memory manually in its destructor.)

Sounds good, done!

Unnecessary const_cast, Buffer::Data() already returns char*.

(Looks like it was existing code but since you're here.)

Could be a std::unique_ptr<char>.

'By default'? :-)

Ditto.

@bnoordhuis Thanks for the review! I think I got everything so far.

LGTM with some comments/questions. Nice work, Anna.

StreamReq::FromObject()? static_cast<Wrap*>(Wrap::FromObject(...)) probably looks redundant and mildly confusing to the casual reader.

StreamResource is-a StreamBase so Write() is available, isn't it? The static_cast looks superfluous to me.

char[] - pointer to array, not pointer to single char. Likewise around line 250.

Space before <. Likewise around line 336.

Is the static_cast necessary here?

Use `DoTryWrite()` to write data to the underlying socket. This does probably not make any difference in performance because the callback is still deferred (for now), but brings TLSWrap in line with other things that write to streams.

Otherwise `this[kCurrentWriteRequest]` is set to a value even if one of the `write` calls throws. This is needed in order not to break tests in a later commit.

Encapsulate stream requests more: - `WriteWrap` and `ShutdownWrap` classes are now tailored to the streams on which they are used. In particular, for most streams these are now plain `AsyncWrap`s and do not carry the overhead of unused libuv request data. - Provide generic `Write()` and `Shutdown()` methods that wrap around the actual implementations, and make *usage* of streams easier, rather than implementing; for example, wrap objects don’t need to be provided by callers anymore. - Use `EmitAfterWrite()` and `EmitAfterShutdown()` handlers to call the corresponding JS handlers, rather than always trying to call them. This makes usage of streams by other C++ code easier and leaner. Also fix up some tests that were previously not actually testing asynchronicity when the comments indicated that they would.

LGTM from a flow / logic perspective. Didn't review the intricacies of C++ in detail, there are certainly more qualified folks around for that.

Also appreciate the extremely detailed comments. Made the review a lot easier. 👍

(One unrelated comment below, more as a general discussion point.)

This makes me wonder, is handle on WriteWrap used at all? I had a brief look at the C++ side and saw nothing. All I'm seeing is it being set/deleted all around. The only place I see it being used at all is process_wrap.

Admittedly I could be missing something more intricate here...

@apapirovski I think it’s only for diagnostic purposes and preventing the handle from being garbage collected; I think at least in the case of JSStreams that could happen because that’s a weak handle and otherwise there might not be any backreference to it?

Trying to think about this... Since the socket & http2 implementations reference the handle on _handle and kHandle, this seems like it would mainly come into play if the stream is socket/session is destroyed? Would the handle still be needed in that case? I don't recall if this would still trigger oncomplete or not.

@apapirovski I think it would call oncomplete, but not synchronously (in the case of libuv streams)?

But generally, it’s not a requirement that streams are only destroyed when they are explicitly closed… http2 objects + JSStream contain no strong Persistents, so they can be garbage collected at any time once there no longer is a reference to them, but that shouldn’t happen during a write, should it?

I guess it's (very) possible that in certain situations the handle is the only thing referencing the stream and the WriteWrap is the only thing referencing the handle. I didn't really think about that originally... Was thinking too literally about its usage.

I figured no harm in putting this to practice... So far, at least as far as http2 is concerned, it seems like removing .handle on WriteWrap & ShutdownWrap is fine, even when running stress tests. I might play around with explicit global.gc() calls and study the code in more detail to understand how useful these references are.

@apapirovski I can’t think of any way in which this would break HTTP/2, yes…

I’m a bit worried removing it might break async_hooks users… but then again, this really isn’t supposed to be public API. :/

🤞

CI: https://ci.nodejs.org/job/node-test-commit/16221/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1287/

Thank you for this :-)

Landed in b2e20b0...0e7b612

Thanks for the reviews!

Use `DoTryWrite()` to write data to the underlying socket. This does probably not make any difference in performance because the callback is still deferred (for now), but brings TLSWrap in line with other things that write to streams. PR-URL: #18676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Otherwise `this[kCurrentWriteRequest]` is set to a value even if one of the `write` calls throws. This is needed in order not to break tests in a later commit. PR-URL: #18676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #18676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Encapsulate stream requests more: - `WriteWrap` and `ShutdownWrap` classes are now tailored to the streams on which they are used. In particular, for most streams these are now plain `AsyncWrap`s and do not carry the overhead of unused libuv request data. - Provide generic `Write()` and `Shutdown()` methods that wrap around the actual implementations, and make *usage* of streams easier, rather than implementing; for example, wrap objects don’t need to be provided by callers anymore. - Use `EmitAfterWrite()` and `EmitAfterShutdown()` handlers to call the corresponding JS handlers, rather than always trying to call them. This makes usage of streams by other C++ code easier and leaner. Also fix up some tests that were previously not actually testing asynchronicity when the comments indicated that they would. PR-URL: #18676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

The root cause is that `req_wrap` is created in `StreamBase::Write` and passed to `TLSWrap::DoWrite`. In the TLS case the object gets disposed and replaced with a new instance, but the caller's pointer is never updated. When the `StreamBase::Write` method returns, it returns a pointer to the freed object to the caller. In some cases when the object memory has already been reused an assert is hit in `WriteWrap::SetAllocatedStorage` because the pointer is non-null. PR-URL: nodejs#18860 Refs: nodejs#18676 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Use `DoTryWrite()` to write data to the underlying socket. This does probably not make any difference in performance because the callback is still deferred (for now), but brings TLSWrap in line with other things that write to streams. PR-URL: nodejs#18676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Otherwise `this[kCurrentWriteRequest]` is set to a value even if one of the `write` calls throws. This is needed in order not to break tests in a later commit. PR-URL: nodejs#18676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#18676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Encapsulate stream requests more: - `WriteWrap` and `ShutdownWrap` classes are now tailored to the streams on which they are used. In particular, for most streams these are now plain `AsyncWrap`s and do not carry the overhead of unused libuv request data. - Provide generic `Write()` and `Shutdown()` methods that wrap around the actual implementations, and make *usage* of streams easier, rather than implementing; for example, wrap objects don’t need to be provided by callers anymore. - Use `EmitAfterWrite()` and `EmitAfterShutdown()` handlers to call the corresponding JS handlers, rather than always trying to call them. This makes usage of streams by other C++ code easier and leaner. Also fix up some tests that were previously not actually testing asynchronicity when the comments indicated that they would. PR-URL: nodejs#18676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The root cause is that `req_wrap` is created in `StreamBase::Write` and passed to `TLSWrap::DoWrite`. In the TLS case the object gets disposed and replaced with a new instance, but the caller's pointer is never updated. When the `StreamBase::Write` method returns, it returns a pointer to the freed object to the caller. In some cases when the object memory has already been reused an assert is hit in `WriteWrap::SetAllocatedStorage` because the pointer is non-null. PR-URL: nodejs#18860 Refs: nodejs#18676 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Marking this as dont-land on LTS because it causes #19562, but if it becomes desirable we could probably backport this either together with #19551 or a smaller custom fix

Use `DoTryWrite()` to write data to the underlying socket. This does probably not make any difference in performance because the callback is still deferred (for now), but brings TLSWrap in line with other things that write to streams. PR-URL: nodejs#18676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Otherwise `this[kCurrentWriteRequest]` is set to a value even if one of the `write` calls throws. This is needed in order not to break tests in a later commit. PR-URL: nodejs#18676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#18676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Encapsulate stream requests more: - `WriteWrap` and `ShutdownWrap` classes are now tailored to the streams on which they are used. In particular, for most streams these are now plain `AsyncWrap`s and do not carry the overhead of unused libuv request data. - Provide generic `Write()` and `Shutdown()` methods that wrap around the actual implementations, and make *usage* of streams easier, rather than implementing; for example, wrap objects don’t need to be provided by callers anymore. - Use `EmitAfterWrite()` and `EmitAfterShutdown()` handlers to call the corresponding JS handlers, rather than always trying to call them. This makes usage of streams by other C++ code easier and leaner. Also fix up some tests that were previously not actually testing asynchronicity when the comments indicated that they would. PR-URL: nodejs#18676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The root cause is that `req_wrap` is created in `StreamBase::Write` and passed to `TLSWrap::DoWrite`. In the TLS case the object gets disposed and replaced with a new instance, but the caller's pointer is never updated. When the `StreamBase::Write` method returns, it returns a pointer to the freed object to the caller. In some cases when the object memory has already been reused an assert is hit in `WriteWrap::SetAllocatedStorage` because the pointer is non-null. PR-URL: nodejs#18860 Refs: nodejs#18676 Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 9, 2018

addaleax added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 9, 2018

addaleax commented Feb 9, 2018

View reviewed changes

addaleax force-pushed the writewrap-red branch from d0a51b6 to 94543e0 Compare February 9, 2018 20:39

bnoordhuis reviewed Feb 10, 2018

View reviewed changes

bnoordhuis approved these changes Feb 13, 2018

View reviewed changes

addaleax added 7 commits February 13, 2018 17:47

tls_wrap: use DoTryWrite()

d26a912

Use `DoTryWrite()` to write data to the underlying socket. This does probably not make any difference in performance because the callback is still deferred (for now), but brings TLSWrap in line with other things that write to streams.

src: only set JSStreamWrap write req after write()

901a4ba

Otherwise `this[kCurrentWriteRequest]` is set to a value even if one of the `write` calls throws. This is needed in order not to break tests in a later commit.

test: make sure WriteWrap tests are actually async

6ea19a5

[squash] fix stupid memory leak, un-tank performance

98de11a

[squash] bnoordhuis nits

ba18a7a

[squash] bnoordhuis suggestion: simplify memory management a bit

a852cc6

apapirovski approved these changes Feb 13, 2018

View reviewed changes

[squash] review comments

a9cdc4b

addaleax force-pushed the writewrap-red branch from 24f33d5 to a9cdc4b Compare February 13, 2018 19:14

addaleax mentioned this pull request Feb 13, 2018

src: prevent persistent handle resource leaks #18656

Merged

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 13, 2018

jasnell approved these changes Feb 13, 2018

View reviewed changes

addaleax closed this Feb 14, 2018

addaleax deleted the writewrap-red branch February 14, 2018 10:03

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 14, 2018

addaleax added a commit that referenced this pull request Feb 14, 2018

test: make sure WriteWrap tests are actually async

0ed9ea8

PR-URL: #18676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

kfarnung mentioned this pull request Feb 19, 2018

tls: use after free in tls_wrap #18860

Merged

2 tasks

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

addaleax removed the backport-requested-v9.x label Feb 27, 2018

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

addaleax added dont-land-on-v6.x labels Mar 24, 2018

src: refactor WriteWrap and ShutdownWrap by addaleax · Pull Request #18676 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: refactor WriteWrap and ShutdownWrap - #18676

src: refactor WriteWrap and ShutdownWrap#18676addaleax wants to merge 8 commits intonodejs:masternodejs/node:masterfrom addaleax:writewrap-redaddaleax/node:writewrap-redCopy head branch name to clipboard

Conversation

addaleax commented Feb 9, 2018

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Feb 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Feb 10, 2018

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Feb 13, 2018

src: refactor WriteWrap and ShutdownWrap#18676
addaleax wants to merge 8 commits into
nodejs:masterfrom
addaleax:writewrap-red

addaleax commented Feb 9, 2018 •
edited
Loading