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

Add msh3 Support #8517

Closed
wants to merge 40 commits into from
Closed

Add msh3 Support #8517

wants to merge 40 commits into from

Conversation

nibanks
Copy link
Contributor

@nibanks nibanks commented Feb 26, 2022

This PR adds a new experimental, cross-platform HTTP/3 option. It leverages msh3 which is an experimental/PoC HTTP/3 library on top of msquic.

Manually tested on Linux (GitHub Codespaces) and on Windows 11 against several public HTTP/3 hosts (google.com, cloudflare.com, outlook.office.com).

@dfandrich
Copy link
Collaborator

@dfandrich dfandrich commented Feb 26, 2022

@nibanks
Copy link
Contributor Author

@nibanks nibanks commented Feb 26, 2022

msquic is officially supported. msh3 is currently just a personal project of mine (~1000 LoC), with a hope of possibly getting it officially supported in the future. But per my understanding, all HTTP/3 code is considered experimental currently anyways, so there is no guarantee of support for the code yet.

lib/vquic/msh3.c Outdated Show resolved Hide resolved
@bagder bagder added the HTTP/3 label Feb 26, 2022
@bagder
Copy link
Member

@bagder bagder commented Feb 26, 2022

@nibanks try make checksrc to get some complaints on code style... They can also be seen in several of the failed CI builds. example

docs/HTTP3.md Show resolved Hide resolved
@bagder
Copy link
Member

@bagder bagder commented Mar 7, 2022

I installed msh3, msquic and boringssl from current git right now and built curl to use them. Then I ran:

./src/curl --http3 https://cloudflare.com/ -v -o dump

... to find that dump ends up a file that is 100360 bytes big, consisting of only binary zeroes. Maybe something related to no body at all since this is just a 301 redirect?

This is on an updated Debian unstable x86_64 machine where curl -V says this:

curl 7.82.1-DEV (x86_64-pc-linux-gnu) libcurl/7.82.1-DEV BoringSSL zlib/1.2.11 brotli/1.0.9 zstd/1.4.8 libidn2/2.3.2 libpsl/0.21.0 (+libidn2/2.3.0) nghttp2/1.43.0 msh3/0.0.1 librtmp/2.3 libgsasl/1.10.0 OpenLDAP/2.4.59
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher gophers http https imap imaps ldap ldaps mqtt pop3 pop3s rtmp rtsp smb smbs smtp smtps telnet tftp 
Features: alt-svc AsynchDNS brotli Debug gsasl HSTS HTTP2 HTTP3 HTTPS-proxy IDN IPv6 Largefile libz NTLM NTLM_WB PSL SSL TrackMemory UnixSockets zstd

@nibanks
Copy link
Contributor Author

@nibanks nibanks commented Mar 7, 2022

... to find that dump ends up a file that is 100360 bytes big, consisting of only binary zeroes. Maybe something related to no body at all since this is just a 301 redirect?

I'll try to investigate when I get some time.

@bagder
Copy link
Member

@bagder bagder commented Mar 24, 2022

As this work is a totally separate build option that you enable and use entirely on purpose and with some dedication, I wouldn't mind merging this early - if it would help the effort - even with flaws and early "warts", if the expectation is that they will be worked on moving forward. I do appreciate how having the foundation landed can help getting more contributors and helpers to get to a starting place easier.

You think that makes sense @nibanks ?

@nibanks
Copy link
Contributor Author

@nibanks nibanks commented Mar 24, 2022

You think that makes sense @nibanks ?

Yeah, it does @bagder. But at the same time, I don't want to just drop some crap on you and leave. I want to give you something that works, and can be taken up by others (if they want). I don't know how much time I will be able to dedicate in the long run to help (beyond official support for msquic and occasional work on msh3).

I've done more work on the msh3 side to onboard CI to run my test tool against 3 servers known to support (different) HTTP/3 solutions. They seem to work just fine, which makes me think the issue is my new code on the curl side.

@nibanks
Copy link
Contributor Author

@nibanks nibanks commented Apr 9, 2022

@bagder things should be good now. Found/fixed the bug you reported about all-zero data size. One interesting thing I found is that the curl thread to drain receives is much slower than the callbacks from msh3. For instance, if you define DEBUG_HTTP3 in the code and run curl --http3 https://www.cloudflare.com/ -o dump you will see the msh3 code spit out logs indicating everything is downloaded instantly, but curl will slowly drain the local receive buffer we've cached, waiting for curl to drain. IMO, that doesn't need to be fixed in this PR though.

@nibanks
Copy link
Contributor Author

@nibanks nibanks commented Apr 9, 2022

capture

@nibanks
Copy link
Contributor Author

@nibanks nibanks commented Apr 9, 2022

I think I figured out at least part of the slow curl thread issue. I had to implement Curl_quic_data_pending to indicate I still had more data for curl (commit to come). But I'm struggling to understand the receive close path. For instance, when I run curl --http3 https://msquic.net/iisstart.png -v, at the end of the logs, I get:

...
* Curl_quic_data_pending
* msh3_stream_recv 102400
* returned 99710 bytes of data
* returned 99710 total bytes in recv
< %PNG
< →
* Curl_quic_data_pending
* msh3_getsock
* msh3_stream_recv 102400
* returned 0 total bytes in recv
* Curl_quic_data_pending
* transfer closed with 99710 bytes remaining to read
* Curl_quic_done
* Connection #0 to host msquic.net left intact
curl: (18) transfer closed with 99710 bytes remaining to read   <===== WHY?

As you can see from logs above, we are returning that data to curl: returned 99710 total bytes in recv. Why does it think it was closed with remaining bytes?

@nibanks
Copy link
Contributor Author

@nibanks nibanks commented Apr 9, 2022

After trying to read through the curl code that calls h3, I still can't figure it out. I'm about as far as I can get for now without help.

@bagder
Copy link
Member

@bagder bagder commented Apr 9, 2022

I appreciate how you might run into some curl peculiarity here as the h3 layering is not always crystal clear or even totally sensible. Let me know if you get stuck for real and I can give it a shot from my end.

@bagder
Copy link
Member

@bagder bagder commented Apr 9, 2022

After trying to read through the curl code that calls h3, I still can't figure it out. I'm about as far as I can get for now without help.

Still this issue you mean?

@nibanks
Copy link
Contributor Author

@nibanks nibanks commented Apr 9, 2022

Still this issue you mean?

Yes. I don't understand how I'm supposed to correctly indicate stream closure to curl. AFAICT, the code looks correct, but at least in some scenarios curl says it was terminated without all the data.

@bagder
Copy link
Member

@bagder bagder commented Apr 9, 2022

I get build errors on Linux:

Making all in lib
  CC       vauth/libcurl_la-cleartext.lo
  CC       vauth/libcurl_la-cram.lo
  CC       vauth/libcurl_la-digest.lo
  CC       vauth/libcurl_la-gsasl.lo
  CC       vauth/libcurl_la-ntlm.lo
  CC       vauth/libcurl_la-oauth2.lo
  CC       vauth/libcurl_la-spnego_gssapi.lo
In file included from ../lib/urldata.h:136,
                 from vauth/cleartext.c:32:
../lib/http.h:179:3: error: expected specifier-qualifier-list before ‘alignas’
  179 |   alignas(16) pthread_mutex_t mutex;
      |   ^~~~~~~
../lib/http.h:178:8: error: struct has no members [-Wpedantic]
  178 | struct posix_lock {
      |        ^~~~~~~~~~
../lib/http.h:181:19: error: unknown type name ‘posix_lock’
  181 | #define msh3_lock posix_lock
      |                   ^~~~~~~~~~
../lib/http.h:276:3: note: in expansion of macro ‘msh3_lock’
  276 |   msh3_lock recv_lock;
      |   ^~~~~~~~~
In file included from ../lib/urldata.h:136,
                 from vauth/ntlm.c:36:
../lib/http.h:179:3: error: expected specifier-qualifier-list before ‘alignas’
  179 |   alignas(16) pthread_mutex_t mutex;
      |   ^~~~~~~
../lib/http.h:178:8: error: struct has no members [-Wpedantic]
  178 | struct posix_lock {
      |        ^~~~~~~~~~
../lib/http.h:181:19: error: unknown type name ‘posix_lock’
  181 | #define msh3_lock posix_lock
      |                   ^~~~~~~~~~
../lib/http.h:276:3: note: in expansion of macro ‘msh3_lock’
  276 |   msh3_lock recv_lock;

@nibanks
Copy link
Contributor Author

@nibanks nibanks commented Apr 10, 2022

Is this PR good to go now? Do you still want me to squash rebase first? Anything else?

@bagder
Copy link
Member

@bagder bagder commented Apr 10, 2022

I think it is good to go. I will squash it on merge myself, no need for you to do it.

@bagder
Copy link
Member

@bagder bagder commented Apr 10, 2022

Thanks!

@nibanks nibanks deleted the add-msh3 branch Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HTTP/3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants