Skip to content

src: minor refactoring to StreamBase #17564

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.

Sign up for GitHub

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

Closed
addaleax wants to merge 4 commits into from
Closed

src: minor refactoring to StreamBase #17564

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e88eba
src: remove `StreamResourc::Cast()`
addaleax Dec 9, 2017
cf96acc
src: minor refactoring to StreamBase writes
addaleax Dec 9, 2017
2c08345
[squash] make CI happy
addaleax Dec 9, 2017
0f28644
test: expand test-https-keep-alive-large-write
addaleax Dec 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
[squash] make CI happy
  • Loading branch information
@addaleax
addaleax committed Dec 9, 2017
commit 2c0834501533a6be158583719999f31f972848a8
1 change: 0 additions & 1 deletion src/tls_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy Markdown
Contributor

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

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

@addaleax addaleax Dec 9, 2017

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
Copy Markdown
Contributor

@apapirovski apapirovski Dec 9, 2017 •
edited
Loading

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

@addaleax addaleax Dec 9, 2017

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
Copy Markdown
Contributor

@apapirovski apapirovski Dec 9, 2017 •
edited
Loading

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

@addaleax addaleax Dec 10, 2017

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
Copy Markdown
Contributor

@apapirovski apapirovski Dec 10, 2017

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

@addaleax addaleax Dec 10, 2017

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
Copy Markdown
Contributor

@apapirovski apapirovski Dec 11, 2017

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

@addaleax addaleax Dec 11, 2017

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…

}


Expand Down