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 and HTTP/2 upload handling fixes #11342

Closed
wants to merge 1 commit into from

Conversation

icing
Copy link
Contributor

@icing icing commented Jun 19, 2023

  • fixes curl consumes 100% CPU when sending a file with -F #11242 where 100% CPU on uploads was reported
  • fixes possible stalls on last part of a request body when that information could not be fully send on the connection due to an EAGAIN
  • applies the same EAGAIN handling to HTTP/2 proxying

- fixes curl#11242 where 100% CPU on uploads was reported
- fixes possible stalls on last part of a request body when
  that information could not be fully send on the connection
  due to an EAGAIN
- applies the same EGAIN handling to HTTP/2 proxying
@bagder bagder closed this in 65937f0 Jun 20, 2023
@bagder
Copy link
Member

bagder commented Jun 20, 2023

Thanks!

bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
- fixes curl#11242 where 100% CPU on uploads was reported
- fixes possible stalls on last part of a request body when
  that information could not be fully send on the connection
  due to an EAGAIN
- applies the same EGAIN handling to HTTP/2 proxying

Reported-by: Sergey Alirzaev
Fixed curl#11242
Closes curl#11342
if(len < stream->upload_blocked_len) {
/* Did we get called again with a smaller `len`? This should not
* happend. We are not prepared to handle that. */
failf(data, "HTTP/2 send again with decreased length");

Choose a reason for hiding this comment

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

Hi @icing 👋 Where could this unhandle-able condition originate (outside of curl)?

There are a few cases of this error popping up, where downgrading to HTTP/1 worked (GitLab.com customer confirms) and increasing buffer size was considered:

I see that this code is duplicated in lib/cf-h2-proxy.c, which I take to mean Cloudflare HTTP/2. However, I don't see CF being involved when tracerouteing the repo sources in the above-linked examples. CF hosts GitLab.com, though.

I'm wondering whether for example firewalls or proxy configs should be looked at to debug this. Thanks for any hints!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @katrinleinweber, thanks for linking the details. Clearly, something is going wrong here. (btw. cf is our internal shortener of connection filter). From the lines I see that you are operating on 8.2.0.

I have to investigate what may be causing this. The check is there to rather fail than proceed with an upload that looks gone wrong. But, as usual, there seem to be wrong assumptions involved.

Choose a reason for hiding this comment

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

Thanks for looking into this, @icing!

you are operating on 8.2.0

No, I just wanted to comment at the source. 8.2.1 seems to have moved the same code downwards a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@katrinleinweber I tried in many ways to trigger this on my machine, but no success. I now suspect that this only happens on 32bit systems. Do you know of any reports on 64bit archs?

Choose a reason for hiding this comment

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

I recently encountered this error with a git push to github.com. I'm not behind any proxies, and had the same result when I used a VPN to connect to an alternative github.com endpoint in another country.

I opened a support ticket with GitHub and they referenced this discussion and suggested I share my experience here. I am using 64 bit git.exe on Windows, version 2.42.0.windows.1.

I have captured a trace log with:

export GIT_TRACE_PACKET=1
export GIT_TRACE=1
export GIT_CURL_VERBOSE=1

I am no longer able to replicate the issue locally (temporarily turning off git compression tweaked things enough that the problem went away), but in case the logs are useful, I have attached them here (git_connect.log.txt). Short log:

mcdurdin@THARK MINGW64 /c/Projects/keyman/app (chore/core/9722-rename-km_kbp_-to_km_core_)
$ git push -u origin chore/core/9722-rename-km_kbp_-to_km_core_ 
Enumerating objects: 399, done.
Counting objects: 100% (399/399), done.
Delta compression using up to 8 threads
Compressing objects: 100% (264/264), done.
Writing objects: 100% (267/267), 63.66 KiB | 1.27 MiB/s, done.
Total 267 (delta 230), reused 0 (delta 0), pack-reused 0
error: RPC failed; curl 16 HTTP/2 send again with decreased length
send-pack: unexpected disconnect while reading sideband packet
fatal: the remote end hung up unexpectedly
Everything up-to-date

In the log, I find the Send header git trace immediately before the failure curious -- why would git be sending a zero-byte header?

14:54:19.988680 http.c:792              => Send header, 0000000000 bytes (0x00000000)

Copy link
Member

Choose a reason for hiding this comment

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

I recently encountered this error with a git push to github.com. I'm not behind any proxies, and had the same result when I used a VPN to connect to an alternative github.com endpoint in another country.

I opened a support ticket with GitHub and they referenced this discussion and suggested I share my experience here. I am using 64 bit git.exe on Windows, version 2.42.0.windows.1.

The issue was fixed in curl 8.3.0 released in September. If you are using the latest Git for Windows it was released in August so unless it has an auto-update of components then it is not using curl 8.3.0.

icing added a commit to icing/curl that referenced this pull request Sep 5, 2023
- refs curl#11342 where errors with git https interactions
  were observed
- problem was caused by 1st sends of size larger than 64KB
  which resulted in later retries of 64KB only
- limit sending of 1st block to 64KB
- adjust h2/h3 filters to cope with parsing the HTTP/1.1
  formatted request in chunks

- introducing Curl_nwrite() as companion to Curl_write()
  for the many cases where the sockindex is already known
bagder pushed a commit that referenced this pull request Sep 5, 2023
- refs #11342 where errors with git https interactions
  were observed
- problem was caused by 1st sends of size larger than 64KB
  which resulted in later retries of 64KB only
- limit sending of 1st block to 64KB
- adjust h2/h3 filters to cope with parsing the HTTP/1.1
  formatted request in chunks

- introducing Curl_nwrite() as companion to Curl_write()
  for the many cases where the sockindex is already known

Fixes #11342 (again)
Closes #11803
@icing
Copy link
Contributor Author

icing commented Sep 5, 2023

@katrinleinweber fix was merged into master for release next week.

@katrinleinweber
Copy link

Thank you, @icing! I wasn't able to obtain more details about the 32/64bit detail, sorry.

@icing
Copy link
Contributor Author

icing commented Sep 6, 2023

Thank you, @icing! I wasn't able to obtain more details about the 32/64bit detail, sorry.

Found a way to reproduce in a test case. The architecture, turned out, did not matter. It was just the right amount of headers and POST data plus an EWOULDBLOCK at the right moment that triggered it.

ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
- fixes curl#11242 where 100% CPU on uploads was reported
- fixes possible stalls on last part of a request body when
  that information could not be fully send on the connection
  due to an EAGAIN
- applies the same EGAIN handling to HTTP/2 proxying

Reported-by: Sergey Alirzaev
Fixed curl#11242
Closes curl#11342
ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
- refs curl#11342 where errors with git https interactions
  were observed
- problem was caused by 1st sends of size larger than 64KB
  which resulted in later retries of 64KB only
- limit sending of 1st block to 64KB
- adjust h2/h3 filters to cope with parsing the HTTP/1.1
  formatted request in chunks

- introducing Curl_nwrite() as companion to Curl_write()
  for the many cases where the sockindex is already known

Fixes curl#11342 (again)
Closes curl#11803
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

curl consumes 100% CPU when sending a file with -F
5 participants