8000 fix: allow ClientRequest responses to be throttled by VerteDinde · Pull Request #25531 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: allow ClientRequest responses to be throttled #25531

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

Jump to bottom
Merged
merged 5 commits into from
Oct 6, 2020
Merged

fix: allow ClientRequest responses to be throttled #25531

merged 5 commits into from
Oct 6, 2020

Conversation

VerteDinde
Copy link
Member
@VerteDinde VerteDinde commented Sep 18, 2020 •
edited by codebytere
Loading

Description of Change

Closes #25130.

Previously, when making a client request, the IncomingMessage class was implementing the Readable stream property correctly. However, its buffer will get filled without stopping by the SimpleURLLoaderWrapper.

This PR passes up a callback from the SimpleURLLoaderWrapper layer to the IncomingMessage class. This callback allows the IncomingMessage class to properly stop the readable stream when it is full and needs the request to pause, and then resume the stream when appropriate.

Checklist

Release Notes

Notes: Fixed an issue where net.request would continue downloading data even when the consuming stream was throttled.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Sep 18, 2020
@VerteDinde VerteDinde requested a review from nornagon September 18, 2020 17:00
@VerteDinde VerteDinde force-pushed the net-throttle branch 2 times, most recently from e55c02c to 4010f83 Compare September 18, 2020 17:51
nornagon
nornagon approved these changes Sep 18, 2020
View reviewed changes
Copy link
Contributor
@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

looks good to me!

@nornagon
Copy link
Contributor

I'd rewrite the release notes a little to not mention internal C++ class names. Maybe something like "Fixed an issue where net.request would continue downloading data even when the consuming stream was throttled"?

@VerteDinde
Copy link
Member Author

@nornagon Definitely! Will make that change now

@nornagon
Copy link
Contributor

Interesting! It looks like this test is failing on Linux, even though it's succeeding on Mac. I wonder if the kernel buffers are bigger on our CI machine?

I just checked on my Ubuntu VM and I found:

$ cat /proc/sys/net/ipv4/tcp_rmem
4096    131072  6291456
$ cat /proc/sys/net/ipv4/tcp_wmem
4096    16384   4194304

these three numbers are min/default/max size of the kernel's read and write buffers, respectively. The max size there is 6MB in the read buffer and 4MB in the write buffer, for a total of 10MB, which is roughly equal to the 9 chunks that the server successfully manages to send.

I think it would make sense to solve this by changing the expectation in the test to be <= 10 chunks, and increase the timeout to 1 second (just to be sure that if it was going to send, like, 100 chunks, then we'd definitely get >10). It's not a perfect test—I wish I could think of a way to test this without a setTimeout—but it'll at least pass.

@electron-cation 8000 electron-cation bot removed the new-pr 🌱 PR opened recently label Sep 19, 2020
zcbenz
zcbenz reviewed Sep 23, 2020
View reviewed changes
@codebytere
Copy link
Member

@VerteDinde looks like this is failing:

net module IncomingMessage API should correctly throttle an incoming stream

@zcbenz zcbenz merged commit 6356cd4 into master Oct 6, 2020
@release-clerk
Copy link
release-clerk bot commented Oct 6, 2020

Release Notes Persisted

Fixed an issue where net.request would continue downloading data even when the consuming stream was throttled.

@zcbenz zcbenz deleted the net-throttle branch October 6, 2020 00:47
@ckerr ckerr mentioned this pull request Feb 24, 2021
Closed
3 tasks
@kyrylo-hrechykhin kyrylo-hrechykhin mentioned this pull request May 10, 2022
Closed
3 tasks
@kyrylo-hrechykhin kyrylo-hrechykhin mentioned this pull request Sep 26, 2022
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nornagon nornagon nornagon approved these changes

@zcbenz zcbenz zcbenz approved these changes

@codebytere codebytere codebytere approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ClientRequest responses cannot be throttled
4 participants
@VerteDinde @nornagon @codebytere @zcbenz
0