src: minor refactoring to StreamBase #17564

addaleax · 2017-12-09T12:25:08Z

src: remove StreamResourc::Cast()

This is not necessary since C++ already has static_cast
as a proper way to cast inside a class hierarchy.
src: minor refactoring to StreamBase writes

Instead of having per-request callbacks, always call a callback
on the StreamBase instance itself for WriteWrap and ShutdownWrap.

This makes WriteWrap cleanup consistent for all stream classes,
since the after-write callback is always the same now.

If special handling is needed for writes that happen to a sub-class,
AfterWrite can be overridden by that class, rather than that
class providing its own callback (e.g. updating the write
queue size for libuv streams).

If special handling is needed for writes that happen on another
stream instance, the existing after_write_cb() callback
is used for that (e.g. custom code after writing to the
transport from a TLS stream).

As a nice bonus, this also makes WriteWrap and ShutdownWrap
instances slightly smaller.

Checklist

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

Affected core subsystem(s)

src/stream_base

~~CI: https://ci.nodejs.org/job/node-test-commit/14692/ (edit: looks like some failures are related)~~

This is not necessary since C++ already has `static_cast` as a proper way to cast inside a class hierarchy.

Instead of having per-request callbacks, always call a callback on the `StreamBase` instance itself for `WriteWrap` and `ShutdownWrap`. This makes `WriteWrap` cleanup consistent for all stream classes, since the after-write callback is always the same now. If special handling is needed for writes that happen to a sub-class, `AfterWrite` can be overridden by that class, rather than that class providing its own callback (e.g. updating the write queue size for libuv streams). If special handling is needed for writes that happen on another stream instance, the existing `after_write_cb()` callback is used for that (e.g. custom code after writing to the transport from a TLS stream). As a nice bonus, this also makes `WriteWrap` and `ShutdownWrap` instances slightly smaller.

addaleax · 2017-12-09T22:48:09Z

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

apapirovski · 2017-12-09T22:51:05Z

src/tls_wrap.cc

@@ -658,7 +658,6 @@ int TLSWrap::DoWrite(WriteWrap* w,
 void TLSWrap::OnAfterWriteImpl(WriteWrap* w, int status, void* ctx) {
  TLSWrap* wrap = static_cast<TLSWrap*>(ctx);
  wrap->EncOutAfterWrite(w, status);
-  wrap->UpdateWriteQueueSize();


It's been a while since I've worked on this but does this still function correctly in terms of updating the write queue size after the full write completes?

@apapirovski I’ve thought about pinging you but didn’t see you around in IRC ;)

I would assume so, or at least assume that something in our suite tests that? Do you have an example of code to test this against?

Also, I have to admit that I’m still not 100 % clear on the semantics inside Socket.prototype._onTimeout … why do we care about the previous write queue size? If we do, why don’t we track that in JS rather than from C++? It’s very hard to follow the data flow for this property since it’s updated at some parts in the code, but it doesn’t really get clear which parts those are…

There's a reason that was a bug in core for a great many years... 😆 (It got exacerbated when keep-alive timeouts were introduced and made to work properly but it was a major issue even before then, it just took a much longer write — over 120s — to discover it.)

Basically, since we hand off the write to C++, the JS side doesn't really know how the write is progressing while it's actually getting written to the socket. To get around that, the C++ code should only update the write state when it first starts and when it fully finishes (0 left). In between that, the JS code (within _onTimeout) is allowed to check on the C++ end to determine whether there has been any progress since the last time _writeQueueSize changed. If not, we're dealing with a timeout. If yes, it's just a long running write.

(Also, the current architecture around this is meant to prevent the C++ side from constantly updating JS objects with new info, so the _onTimeout checks itself so that these checks only happen very infrequently.)

Does that make it a bit clearer?

it just took a much longer write — over 120s — to discover it

Do we have/need/want any tests for that? I don’t think any of the tests take that long…

Does that make it a bit clearer?

Yes, it does, thank you. 💙

What would you think about the following?

Keep track of the current writeQueueSize in JS when writing to a socket, e.g. storing it as lastKnownWriteQueueSize

In the timeout, check whether the current and the last known WQS match; if they do, emit the timeout, if not, update lastKnownWriteQueueSize with the current value and reschedule the timeout

Letting C++ update the variable whenever it sees fit (In particular, we could and probably should to that through an Uint32Array rather than )

Does that sound okay? I think that might be a bit easier to keep track off…

Do we have/need/want any tests for that? I don’t think any of the tests take that long…

The test that failed earlier tests for it, it just modifies the timeout value to make it shorter.

The thing it doesn't test for is whether the writeQueueSize gets reset down to 0 after the write is done. Maybe you could expand that test as part of this PR?

What would you think about the following?

I feel like I would prefer to keep the size updating to a minimum since it's not used in C++ and the timeouts aren't frequent. I would rather have a slightly slower _onTimeout that needs to make an extra check than do more work during all the other regular writes that won't ever trigger a timeout. (I don't think calling BIO_pending constantly is free.)

I wonder if there's a middle road here. I'll try to think on it tonight.

What if we do the initial set in JS and then the reset to 0 in JS? Then only call the C++ version when _onTimeout occurs. I think that's the best of both worlds?

I’m not sure I follow – is the idea that we only track the write queue size on the JS object, refreshing it when writes happen or finish in JS, and in _onTimeout?

I'll dig into it now.

Yay, thanks! :)

I’m not sure I follow – is the idea that we only track the write queue size on the JS object, refreshing it when writes happen or finish in JS, and in _onTimeout?

Yeah, basically just don't manage writeQueueSize in C++ at all. If _onTimeout requests it then just return the updated value and then the JS can actually update the reference on the object.

Ok, yeah, that sounds fine to me :)

FWIW this might be a bit tricker than I had originally imagined bc it's used by all the different wraps, like TTY, Pipe, etc. I might be changing quite a bit more about how all of this works than I had originally intended.

I kinda like hearing that, tbh ;) This PR wouldn’t have been the last refactoring by far, hopefully…

Check that `serverConnectionHandle.writeQueueSize === 0` after a large write finished.

addaleax · 2017-12-09T23:44:19Z

I don’t think you can run enough CIs on this change.

CI: https://ci.nodejs.org/job/node-test-commit/14714/

addaleax · 2017-12-11T19:12:28Z

@apapirovski Just to know where this is heading, how does your upcoming PR relate to this one? Does it depend on it/replace it? If it’s the former, could you (or maybe @jasnell ?) review it?

apapirovski · 2017-12-11T19:15:07Z

@addaleax It depends on it to an extent. I'm still looking at a few possible directions given that writeQueueSize has been around for a while and I'm not sure if anyone is depending on it. Want to avoid making it semver-major if possible.

Will review this PR today.

apapirovski

Everything SGTM. This is like my 3rd pass through the code but I would like to defer to @jasnell or someone else that's more familiar with this implementation. I'm still discovering new things in the *_wrap / stream_base code every day.

BridgeAR · 2017-12-12T21:39:41Z

PTAL @nodejs/streams

This is not necessary since C++ already has `static_cast` as a proper way to cast inside a class hierarchy. PR-URL: nodejs#17564 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Instead of having per-request callbacks, always call a callback on the `StreamBase` instance itself for `WriteWrap` and `ShutdownWrap`. This makes `WriteWrap` cleanup consistent for all stream classes, since the after-write callback is always the same now. If special handling is needed for writes that happen to a sub-class, `AfterWrite` can be overridden by that class, rather than that class providing its own callback (e.g. updating the write queue size for libuv streams). If special handling is needed for writes that happen on another stream instance, the existing `after_write_cb()` callback is used for that (e.g. custom code after writing to the transport from a TLS stream). As a nice bonus, this also makes `WriteWrap` and `ShutdownWrap` instances slightly smaller. PR-URL: nodejs#17564 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Check that `serverConnectionHandle.writeQueueSize === 0` after a large write finished. PR-URL: nodejs#17564 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · 2017-12-13T05:46:52Z

Landed in 24b0f67, 453008d, c3bd59b

This is not necessary since C++ already has `static_cast` as a proper way to cast inside a class hierarchy. PR-URL: #17564 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Instead of having per-request callbacks, always call a callback on the `StreamBase` instance itself for `WriteWrap` and `ShutdownWrap`. This makes `WriteWrap` cleanup consistent for all stream classes, since the after-write callback is always the same now. If special handling is needed for writes that happen to a sub-class, `AfterWrite` can be overridden by that class, rather than that class providing its own callback (e.g. updating the write queue size for libuv streams). If special handling is needed for writes that happen on another stream instance, the existing `after_write_cb()` callback is used for that (e.g. custom code after writing to the transport from a TLS stream). As a nice bonus, this also makes `WriteWrap` and `ShutdownWrap` instances slightly smaller. PR-URL: #17564 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Check that `serverConnectionHandle.writeQueueSize === 0` after a large write finished. PR-URL: #17564 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This is not necessary since C++ already has `static_cast` as a proper way to cast inside a class hierarchy. PR-URL: #17564 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Instead of having per-request callbacks, always call a callback on the `StreamBase` instance itself for `WriteWrap` and `ShutdownWrap`. This makes `WriteWrap` cleanup consistent for all stream classes, since the after-write callback is always the same now. If special handling is needed for writes that happen to a sub-class, `AfterWrite` can be overridden by that class, rather than that class providing its own callback (e.g. updating the write queue size for libuv streams). If special handling is needed for writes that happen on another stream instance, the existing `after_write_cb()` callback is used for that (e.g. custom code after writing to the transport from a TLS stream). As a nice bonus, this also makes `WriteWrap` and `ShutdownWrap` instances slightly smaller. PR-URL: #17564 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Check that `serverConnectionHandle.writeQueueSize === 0` after a large write finished. PR-URL: #17564 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-01-22T08:49:23Z

Should this be considered for LTS?

mcollina · 2018-01-22T08:51:17Z

I would consider it only for 8, it will make backporting things easier.

MylesBorins · 2018-02-27T03:05:53Z

based on what @mcollina said I'm setting dont-land-on-v6.x and am officially requesting a backport to v8.x

Instead of having per-request callbacks, always call a callback on the `StreamBase` instance itself for `WriteWrap` and `ShutdownWrap`. This makes `WriteWrap` cleanup consistent for all stream classes, since the after-write callback is always the same now. If special handling is needed for writes that happen to a sub-class, `AfterWrite` can be overridden by that class, rather than that class providing its own callback (e.g. updating the write queue size for libuv streams). If special handling is needed for writes that happen on another stream instance, the existing `after_write_cb()` callback is used for that (e.g. custom code after writing to the transport from a TLS stream). As a nice bonus, this also makes `WriteWrap` and `ShutdownWrap` instances slightly smaller. Backport-PR-URL: #20456 PR-URL: #17564 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Instead of having per-request callbacks, always call a callback on the `StreamBase` instance itself for `WriteWrap` and `ShutdownWrap`. This makes `WriteWrap` cleanup consistent for all stream classes, since the after-write callback is always the same now. If special handling is needed for writes that happen to a sub-class, `AfterWrite` can be overridden by that class, rather than that class providing its own callback (e.g. updating the write queue size for libuv streams). If special handling is needed for writes that happen on another stream instance, the existing `after_write_cb()` callback is used for that (e.g. custom code after writing to the transport from a TLS stream). As a nice bonus, this also makes `WriteWrap` and `ShutdownWrap` instances slightly smaller. PR-URL: nodejs/node#17564 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added 2 commits December 9, 2017 04:47

src: remove StreamResourc::Cast()

3e88eba

This is not necessary since C++ already has `static_cast` as a proper way to cast inside a class hierarchy.

addaleax 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 Dec 9, 2017

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 Dec 9, 2017

addaleax added the wip Issues and PRs that are still a work in progress. label Dec 9, 2017

[squash] make CI happy

2c08345

addaleax removed the wip Issues and PRs that are still a work in progress. label Dec 9, 2017

apapirovski reviewed Dec 9, 2017

View reviewed changes

test: expand test-https-keep-alive-large-write

0f28644

Check that `serverConnectionHandle.writeQueueSize === 0` after a large write finished.

addaleax force-pushed the stream-base-refactor branch from 4069955 to 0f28644 Compare December 9, 2017 23:36

addaleax mentioned this pull request Dec 11, 2017

doc,test: mention Duplex support for TLS #17599

Closed

4 tasks

apapirovski approved these changes Dec 11, 2017

View reviewed changes

addaleax mentioned this pull request Dec 12, 2017

http2: refactor close/destroy for Http2Stream and Http2Session #17406

Closed

4 tasks

jasnell approved these changes Dec 12, 2017

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017

addaleax closed this Dec 13, 2017

addaleax deleted the stream-base-refactor branch December 13, 2017 05:47

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added lts-watch-v6.x labels Jan 22, 2018

MylesBorins added dont-land-on-v6.x and removed lts-watch-v6.x labels Feb 27, 2018

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

BethGriggs added backported-to-v8.x and removed backport-requested-v8.x labels Jul 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: minor refactoring to StreamBase #17564

src: minor refactoring to StreamBase #17564

addaleax commented Dec 9, 2017 •

edited

Loading

addaleax commented Dec 9, 2017 •

edited

Loading

apapirovski Dec 9, 2017

addaleax Dec 9, 2017

apapirovski Dec 9, 2017 •

edited

Loading

addaleax Dec 9, 2017

apapirovski Dec 9, 2017 •

edited

Loading

addaleax Dec 10, 2017

apapirovski Dec 10, 2017

addaleax Dec 10, 2017

apapirovski Dec 11, 2017

addaleax Dec 11, 2017

addaleax commented Dec 9, 2017

addaleax commented Dec 11, 2017

apapirovski commented Dec 11, 2017

apapirovski left a comment •

edited

Loading

BridgeAR commented Dec 12, 2017

addaleax commented Dec 13, 2017

MylesBorins commented Jan 22, 2018

mcollina commented Jan 22, 2018

MylesBorins commented Feb 27, 2018

src: minor refactoring to StreamBase #17564

src: minor refactoring to StreamBase #17564

Conversation

addaleax commented Dec 9, 2017 • edited Loading

Checklist

Affected core subsystem(s)

addaleax commented Dec 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Dec 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Dec 9, 2017 • edited Loading

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 Dec 9, 2017

addaleax commented Dec 11, 2017

apapirovski commented Dec 11, 2017

apapirovski left a comment • edited Loading

Choose a reason for hiding this comment

BridgeAR commented Dec 12, 2017

addaleax commented Dec 13, 2017

MylesBorins commented Jan 22, 2018

mcollina commented Jan 22, 2018

MylesBorins commented Feb 27, 2018

addaleax commented Dec 9, 2017 •

edited

Loading

addaleax commented Dec 9, 2017 •

edited

Loading

apapirovski Dec 9, 2017 •

edited

Loading

apapirovski Dec 9, 2017 •

edited

Loading

apapirovski left a comment •

edited

Loading