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

A paused HTTP/2 stream doesn't actually stop the data #4939

Closed
bagder opened this issue Feb 17, 2020 · 6 comments
Closed

A paused HTTP/2 stream doesn't actually stop the data #4939

bagder opened this issue Feb 17, 2020 · 6 comments
Assignees

Comments

@bagder
Copy link
Member

@bagder bagder commented Feb 17, 2020

I did this

Write an application that makes multiple HTTP/2 streams over the same connection. Return CURL_WRITEFUNC_PAUSE from the write callback when you want to pause a stream.

This only stops the data from being delivered to the application. libcurl will then instead just buffer all the incoming data in memory in an ever-growing realloc()ed buffer until the stream is again unpaused or shut down. This is not intended, expected or documented.

(Pausing a HTTP/2 transfer with curl_easy_pause() has in fact the exact same behavior.)

I expected the following

Data to stop flowing over the wire for the stream until we unpause the stream again.

curl/libcurl version

All

operating system

All

@bagder bagder self-assigned this Feb 17, 2020
bagder added a commit that referenced this issue Feb 17, 2020
Fixes #4939
tatsuhiro-t added a commit to nghttp2/nghttp2 that referenced this issue Feb 18, 2020
tatsuhiro-t added a commit to nghttp2/nghttp2 that referenced this issue Feb 20, 2020
Previously, if automatic window update is enabled (which is default),
after window size is set to 0 by
nghttp2_session_set_local_window_size, once the receiving window is
exhausted, even after window size is increased by
nghttp2_session_set_local_window_size, no more data cannot be
received.  This is because nghttp2_session_set_local_window_size does
not submit WINDOW_UPDATE.  It is only triggered when new data arrives
but since window is filled up, no more data cannot be received, thus
dead lock happens.

This commit fixes this issue.  nghttp2_session_set_local_window_size
submits WINDOW_UPDATE if necessary.

curl/curl#4939
@bagder
Copy link
Member Author

@bagder bagder commented Feb 20, 2020

Here's 4939.c which reproduces the problem.

bagder added a commit that referenced this issue Feb 20, 2020
This changes the HTTP/2 window size to set to the CURLOPT_BUFFERSIZE.

Requires nghttp2 commit b3f85e2daa629
(nghttp2/nghttp2#1444) to work properly, to end
up in the next release after 1.40.0.

Fixes #4939
Closes #4940
@jay
Copy link
Member

@jay jay commented Feb 20, 2020

Ref: https://curl.haxx.se/mail/lib-2020-02/0051.html

Can't we just make the window something more reasonable like 1MB or something?

@bagder
Copy link
Member Author

@bagder bagder commented Feb 21, 2020

Can't we just make the window something more reasonable like 1MB or something?

That would certainly make it a simpler fix. And yeah, maybe just do that for now and then possibly do something else in a future if users really need something more flexible.

bagder added a commit that referenced this issue Feb 21, 2020
This changes the HTTP/2 window size to set to the CURLOPT_BUFFERSIZE.

Requires nghttp2 commit b3f85e2daa629
(nghttp2/nghttp2#1444) to work properly, to end
up in the next release after 1.40.0.

Fixes #4939
Closes #4940
bagder added a commit that referenced this issue Feb 21, 2020
This changes the HTTP/2 window size to set to the CURLOPT_BUFFERSIZE.

Requires nghttp2 commit b3f85e2daa629
(nghttp2/nghttp2#1444) to work properly, to end
up in the next release after 1.40.0.

Fixes #4939
Closes #4940
@jay
Copy link
Member

@jay jay commented Feb 22, 2020

Since I posted that I found #1102 for the history of why we added the large window. I tried that example with NGHTTP2_INITIAL_WINDOW_SIZE, 1MB and 10MB.

time src/curl https://upload.wikimedia.org/wikipedia/commons/d/d6/Seattle_7.jpg -v >/dev/null

NGHTTP2_INITIAL_WINDOW_SIZE:
real 0m27.026s
user 0m0.328s
sys 0m0.848s

1MB:
real 0m6.774s
user 0m0.252s
sys 0m0.280s

10MB:
real 0m5.689s
user 0m0.240s
sys 0m0.400s

(1 << 30):
real 0m5.586s
user 0m0.220s
sys 0m0.488s

Repeated runs deviate by about a second.

Bandwidth and latency affect a lot and I wonder if whatever the alternative is if it's better to pursue that instead.

/cc @afrind

@afrind
Copy link

@afrind afrind commented Feb 24, 2020

I don't know the curl or nghttp2 code, but I assume that when application pauses reading from a stream, curl stops releasing flow control for it. The peer is still allowed to send up to a full stream flow control window. There isn't a good way to revoke stream flow control credit that's already granted.

The purpose of the flow control window is to communicate to the peer the amount you are willing to buffer. Setting it to something smaller than BDP can throttle performance, but setting it too large can be bad too. I think 2^30 is probably higher than any reasonable BDP. Buffering 10MB per stream may not be realistic either depending on how many concurrent streams you use and how much memory you have.

@bagder
Copy link
Member Author

@bagder bagder commented Feb 27, 2020

I think the most sensible approach forward is to set this default value large enough to "basically never cause performance problems", probably meaning a value of like 32 MB.

This will also cause a paused transfer to potentially have to buffer up to 32MB, but I can't see any easy way out of that - other than introducing a new option for such specific applications (saved for another day). Of course we should also make CURLOPT_MAX_RECV_SPEED_LARGE affect the window size, but I will leave that too as a separate exercise.

bagder added a commit that referenced this issue Feb 27, 2020
This reduces the HTTP/2 window size to 32 MB since libcurl might have to
buffer up to this amount of data in memory and yet we don't want it set
lower to potentially impact tranfer performance on high speed networks.

Requires nghttp2 commit b3f85e2daa629
(nghttp2/nghttp2#1444) to work properly, to end
up in the next release after 1.40.0.

Fixes #4939
Closes #4940
bagder added a commit that referenced this issue Feb 27, 2020
This reduces the HTTP/2 window size to 32 MB since libcurl might have to
buffer up to this amount of data in memory and yet we don't want it set
lower to potentially impact tranfer performance on high speed networks.

Requires nghttp2 commit b3f85e2daa629
(nghttp2/nghttp2#1444) to work properly, to end
up in the next release after 1.40.0.

Fixes #4939
Closes #4940
@bagder bagder closed this in 15f5147 Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.