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

HTTP/2 POST: Busy loop after 64k if request not read on server #7821

Closed
steini2000 opened this issue Oct 6, 2021 · 8 comments
Closed

HTTP/2 POST: Busy loop after 64k if request not read on server #7821

steini2000 opened this issue Oct 6, 2021 · 8 comments
Assignees
Labels

Comments

@steini2000
Copy link
Contributor

@steini2000 steini2000 commented Oct 6, 2021

The test program sends a POST request using HTTP/2 and CURLOPT_READFUNCTION.
On the server the request is received, but for testing the server does not read the buffer.

If more then 65535 bytes are returned from the read function (in total, after multiple calls of the read function), the test program goes into a busy loop:

  • curl_multi_poll() returns immediately
  • read function is not called
  • no data on the wire

The situation is not satisfying: Nothing happens, but 100% CPU is used.

The following debug log is written during the busy loop for libcurl < 7.75:
=> Send data, 0 bytes (0x0)
This curl_debug() has been disabled with commit a5bc272.

Not reading the request on the server is only the edge case. When (slowly) reading the request on the server, libcurl sometimes still uses 100% CPU and does nothing.

Same problem when using select() instead of curl_multi_poll().

The problem goes away if using CURL_HTTP_VERSION_1_1:
More then 64k data is sent.

The problem does not appear if server sends status code >= 300:
Info: HTTP error before end of send, stop sending
-> RST_STREAM
If the status code is e.g. 204, problem is there.

The problem does not happen with new Tomcat versions. Most likely the relevant change is in 9.0.18:
When Tomcat writes a final response without reading all of an HTTP/2 request, reset the stream to inform the client that the remaining request body is not required. (markt)
But nevertheless the old Tomcat behaviour is valid and could happen with other HTTP/2 implementations.

Tested versions:
7.68.0 with nghttp2 1.40 on x64
7.79.1 with nghttp2 1.41 on arm
Tomcat 9.0.16, does not happen with 9.0.53

Server not reading the buffer is a valid state (at least temporary), libcurl should not busy loop after 64k. Different behaviour for HTTP 1.1 and HTTP/2.

Test programs and log

@bagder bagder added the HTTP/2 label Oct 6, 2021
@steini2000
Copy link
Contributor Author

@steini2000 steini2000 commented Oct 6, 2021

When returning <= 65535 bytes from the read callback, everything is fine: idle loop. curl_multi_poll() returns after the timeout.

Loading

@bagder
Copy link
Member

@bagder bagder commented Oct 7, 2021

When returning <= 65535 bytes from the read callback

You then mean in total right? Because the way this code works it will never return that large single values from the read callback?

Loading

@steini2000
Copy link
Contributor Author

@steini2000 steini2000 commented Oct 7, 2021

When returning <= 65535 bytes from the read callback

You then mean in total right? Because the way this code works it will never return that large single values from the read callback?

Yes, in total. Using multiple calls of the read callback.

Loading

@bagder
Copy link
Member

@bagder bagder commented Oct 7, 2021

I don't know how to reproduce this. I don't have a tomcat installation and I really can't make myself go down that rabbit hole for this purpose. Any publicly accessible URL that shows the same issue?

Loading

@steini2000
Copy link
Contributor Author

@steini2000 steini2000 commented Oct 7, 2021

Not at the moment. But I should be able to setup one. Please allow some time.
I would feel better if I would not have to post it here. Is it OK to mail it?

Loading

@bagder
Copy link
Member

@bagder bagder commented Oct 7, 2021

Yes, thanks, that would be excellent!

Loading

@bagder
Copy link
Member

@bagder bagder commented Oct 11, 2021

[I received a URL to a test server setup by @steini2000 and it is most useful!]

I think this is a curl bug that is most noticeable because of a server bug. It turns out that the server doesn't enlarge the remote window, so it drains (thus not allowing more data to get sent) but curl doesn't notice (partly because nghttp2_session_want_write() still returns TRUE in this situation) and then busy-loops around the trying to send.

Fixing the busy-loop (by checking the remote window before waiting for socket writability) doesn't make the operation succeed because the server doesn't enlarge the window so there's no permission to send data.

The fact that this behavior is gone in a newer tomcat release could perhaps be a sign that this bug is fixed already.

PR coming up.

Loading

bagder added a commit that referenced this issue Oct 11, 2021
While uploading, check for remote window availability in the getsock
function so that we don't wait for a writable socket if no data can be
sent.

Reported-by: Steini2000 on github
Fixes #7821
@bagder bagder self-assigned this Oct 12, 2021
@steini2000
Copy link
Contributor Author

@steini2000 steini2000 commented Oct 12, 2021

This seems to fix the busy loop. Thanks!

Newer Tomcat versions always send a RST_STREAM when sending a response without reading all of the request. I have not tested without sending a response. There are a lot of changes in Tomcat regarding the flow control window. But this is not important any more: libcurl is prepared!

Loading

@bagder bagder closed this in 1fed8fe Oct 12, 2021
D4v1dH03 added a commit to D4v1dH03/curl that referenced this issue Oct 22, 2021
While uploading, check for remote window availability in the getsock
function so that we don't wait for a writable socket if no data can be
sent.

Reported-by: Steini2000 on github
Fixes curl#7821
Closes curl#7839

runtests: split out ignored tests

Report ignore tests separately from the actual fails.

Don't exit non-zero if test servers couldn't get killed.

Assisted-by: Jay Satiro

Fixes curl#7818
Closes curl#7841

tests: disable test 2043

It uses revoked.badssl.com which now is expired and therefor this now
permafails. We should not use external sites for tests, this test should
be converted to use our own infra.

Closes curl#7845

curl: correct grammar in generated libcurl code

Closes curl#7802

Added a note about OpenSSL > 3.0.0
D4v1dH03 added a commit to D4v1dH03/curl that referenced this issue Oct 22, 2021
1. If writing to a system path if the command is not prefixed with `sudo` it will cause a permission denied error
2. The patched OpenSSL branch has been updated to `openssl-3.0.0+quic` to match upstream OpenSSL version.
3. We should not disable GnuTLS docs.

If you believe my changes are wrong, feel free to request changes.

Updated some commands about `make install`

Make the `sudo` optional because one does not need to prepend `make install` with `sudo` all the time.

http2: make getsock not wait for write if there's no remote window

While uploading, check for remote window availability in the getsock
function so that we don't wait for a writable socket if no data can be
sent.

Reported-by: Steini2000 on github
Fixes curl#7821
Closes curl#7839

runtests: split out ignored tests

Report ignore tests separately from the actual fails.

Don't exit non-zero if test servers couldn't get killed.

Assisted-by: Jay Satiro

Fixes curl#7818
Closes curl#7841

tests: disable test 2043

It uses revoked.badssl.com which now is expired and therefor this now
permafails. We should not use external sites for tests, this test should
be converted to use our own infra.

Closes curl#7845

curl: correct grammar in generated libcurl code

Closes curl#7802

Added a note about OpenSSL > 3.0.0

Fixed the wording about the lib64 folder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants