Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: minor refactoring to StreamBase #17564

Closed
wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Dec 9, 2017

  • 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 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 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 addaleax added the wip Issues and PRs that are still a work in progress. label Dec 9, 2017
@addaleax addaleax removed the wip Issues and PRs that are still a work in progress. label Dec 9, 2017
@addaleax
Copy link
Member Author

addaleax commented Dec 9, 2017

src/tls_wrap.cc Outdated
@@ -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();
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@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…

Copy link
Member

@apapirovski apapirovski Dec 9, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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…

Copy link
Member

@apapirovski apapirovski Dec 9, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, yeah, that sounds fine to me :)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

addaleax commented Dec 9, 2017

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

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

@addaleax
Copy link
Member Author

@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
Copy link
Member

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

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

PTAL @nodejs/streams

@addaleax 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 added a commit to addaleax/node that referenced this pull request Dec 13, 2017
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>
addaleax added a commit to addaleax/node that referenced this pull request Dec 13, 2017
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>
addaleax added a commit to addaleax/node that referenced this pull request Dec 13, 2017
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
Copy link
Member Author

Landed in 24b0f67, 453008d, c3bd59b

@addaleax addaleax closed this Dec 13, 2017
addaleax added a commit that referenced this pull request Dec 13, 2017
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>
addaleax added a commit that referenced this pull request Dec 13, 2017
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>
addaleax added a commit that referenced this pull request Dec 13, 2017
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>
@addaleax addaleax deleted the stream-base-refactor branch December 13, 2017 05:47
@addaleax 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 pushed a commit that referenced this pull request Jan 8, 2018
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>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
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>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
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 MylesBorins mentioned this pull request Jan 10, 2018
@MylesBorins
Copy link
Contributor

Should this be considered for LTS?

@mcollina
Copy link
Member

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

@MylesBorins
Copy link
Contributor

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

MylesBorins pushed a commit that referenced this pull request May 2, 2018
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>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants