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/3: Windows build sends lots of empty UDP datagrams #9086

Closed
tatsuhiro-t opened this issue Jul 3, 2022 · 29 comments
Closed

HTTP/3: Windows build sends lots of empty UDP datagrams #9086

tatsuhiro-t opened this issue Jul 3, 2022 · 29 comments
Assignees
Labels
HTTP/3 h3 or quic related Windows Windows-specific

Comments

@tatsuhiro-t
Copy link
Contributor

tatsuhiro-t commented Jul 3, 2022

I did this

Compile curl windows build (mingw64 works) and contact HTTP/3 server with --http3 option.
You need to either check server log or capture the packets to see the empty UDP datagrams.
The number of UDP datagrams varies, but I observed ~500.

It looks like the empty UDP datagram is sent by this call:

curl/lib/multi.c

Line 1238 in 0defae2

send(s, NULL, 0, 0); /* reset FD_WRITE */

which was introduced via b36442b

If it is TCP specific workaround, maybe we should skip it on UDP sockets?

I expected the following

curl should not send empty UDP datagrams on HTTP/3 connection because such UDP datagram is invalid on QUIC specification.

curl/libcurl version

master branch
[curl -V output]

operating system

@jay jay added HTTP/3 h3 or quic related Windows Windows-specific labels Jul 3, 2022
@jay
Copy link
Member

jay commented Jul 3, 2022

@mback2k

@bagder
Copy link
Member

bagder commented Jul 3, 2022

That's an awful lot of send()s...

@bagder
Copy link
Member

bagder commented Jul 3, 2022

They were added for performance reasons (but not tested for h3 I bet). What exactly happens if we don't send() there?

@mback2k
Copy link
Member

mback2k commented Jul 3, 2022

If we don't send, then we won't get another write-ready event from select if we don't send in between two calls, effectively blocking till the next readiness event. I guess we could try to just use this workaround with TCP, as UDP should always be ready for writing/sending,
but I haven't tested that.

Please see the message of the commit mentioned above for more details why this was necessary.

@bagder
Copy link
Member

bagder commented Jul 4, 2022

If we don't send, then we won't get another write-ready event from select

From WSAEventSelect then I presume? select() and poll() are not edge-trigged, they should return writable for as long as it is writable.

we could try to just use this workaround with TCP, as UDP should always be ready for writing/sending,

Yes we should, because send()ing zero bytes might be an almost no-op with TCP but as @tatsuhiro-t figured out: it clearly is much more than that for UDP.

@bagder
Copy link
Member

bagder commented Jul 4, 2022

It doesn't look very easy to add that check though... Can we back-peddle the use of WSAEventSelect() there ?

@mback2k
Copy link
Member

mback2k commented Jul 4, 2022

If we don't send, then we won't get another write-ready event from select

From WSAEventSelect then I presume? select() and poll() are not edge-trigged, they should return writable for as long as it is writable.

Yes, sorry, I meant WSAEventSelect in this context of course, but since I was responding via the mobile app I didn't have time and context to fully type it out.

@mback2k
Copy link
Member

mback2k commented Jul 4, 2022

It doesn't look very easy to add that check though... Can we back-peddle the use of WSAEventSelect() there ?

We could try to use getsockopt to get a SO_PROTOCOL_INFOW which contains the protocol attached to the socket. But I guess we would then need to cache this somehow to not hurt the performance too much.

Back-peddling the use of WSAEventSelect would bring the return of emulating socketpair with local TCP sockets which was the original reason to implement it this way, see the history of multi.c up to that point and especially #5397.

Edit: we could also try SO_PROTOCOL_INFO or just SO_TYPE.

@tatsuhiro-t
Copy link
Contributor Author

UDP send() could actually return EAGAIN/EWOULDBLOCK. It is not always write ready.

@mback2k
Copy link
Member

mback2k commented Jul 8, 2022

I will try to work on a solution soon.

@mback2k
Copy link
Member

mback2k commented Jul 10, 2022

@tatsuhiro-t would you be able to share the commands you used to build curl with HTTP/3 support for/using MinGW-w64?

@mback2k
Copy link
Member

mback2k commented Jul 10, 2022

I tried to replicate this issue locally using msh3 by @nibanks as HTTP/3 backend, but unfortunately I always get this error for now:

D:\OS\curl\winbuild>..\builds\libcurl-vc-x64-release-dll-ipv6-sspi-schannel-msh3\bin\curl.exe -v --http3 https://www.google.com
*   Trying 142.250.181.228:443...
* can't create msh3 connection
* Closing connection 0
curl: (2) can't create msh3 connection

@gvanem
Copy link
Contributor

gvanem commented Jul 11, 2022

can't create msh3 connection

Look in the Event Log for messages like:
An unrecoverable error occurred while creating a TLS client credentials. The internal error condition is 10013.

Occurs if your Win-10 version is too old or some Registry setting is wrong.

@tatsuhiro-t
Copy link
Contributor Author

@mback2k

./configure --host=x86_64-w64-mingw32 --with-ngtcp2=/path/to/ngtcp2 --with-nghttp3=/path/to/nghttp3 --enable-werror --enable-debug --with-openssl=/path/to/quictls --without-zlib

You might need to specify CPPFLAGS and LDFLAGS to prioritize the header file locations for ngtcp2/nghttp3/openssl libraries compiled for mingw.

@mback2k
Copy link
Member

mback2k commented Jul 11, 2022

@tatsuhiro-t thanks, but I also meant the dependencies.

@tatsuhiro-t
Copy link
Contributor Author

Basically, follow the instruction https://github.com/ngtcp2/ngtcp2#build-from-git
For quictls, ./Configure --cross-compile-prefix=x86_64-w64-mingw32- mingw64
For ngtcp2 and nghttp3, pass --host=x86_64-w64-mingw32 (and --enable-lib-only) to configure script.

@mback2k
Copy link
Member

mback2k commented Jul 12, 2022

Occurs if your Win-10 version is too old or some Registry setting is wrong.

Any tip what needs to be changed to make this work on Win10 then?

Basically, follow the instruction https://github.com/ngtcp2/ngtcp2#build-from-git
For quictls, ./Configure --cross-compile-prefix=x86_64-w64-mingw32- mingw64
For ngtcp2 and nghttp3, pass --host=x86_64-w64-mingw32 (and --enable-lib-only) to configure script.

Thanks, but unfortunately this results in the following error message while compiling curl:

In file included from vquic/ngtcp2.c:35:
D:/msys64/opt/ngtcp2/include/ngtcp2/ngtcp2_crypto_openssl.h:73:5: error: unknown type name 'OSSL_ENCRYPTION_LEVEL'
   73 |     OSSL_ENCRYPTION_LEVEL ossl_level);
      |     ^~~~~~~~~~~~~~~~~~~~~
D:/msys64/opt/ngtcp2/include/ngtcp2/ngtcp2_crypto_openssl.h:82:15: error: unknown type name 'OSSL_ENCRYPTION_LEVEL'
   82 | NGTCP2_EXTERN OSSL_ENCRYPTION_LEVEL
      |               ^~~~~~~~~~~~~~~~~~~~~
vquic/ngtcp2.c: In function 'quic_init_ssl':
vquic/ngtcp2.c:317:3: error: implicit declaration of function 'SSL_set_quic_use_legacy_codepoint' [-Wimplicit-function-declaration]
  317 |   SSL_set_quic_use_legacy_codepoint(qs->ssl, 0);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

./configure found all the dependencies as can be seen here:

configure: Configured to build curl/libcurl:

  Host setup:       x86_64-w64-mingw32
  Install prefix:   /mingw64
  Compiler:         x86_64-w64-mingw32-gcc
   CFLAGS:          -Werror-implicit-function-declaration -O2 -Wno-system-headers
   CPPFLAGS:        -isystem D:/msys64/mingw64/include -isystem D:/msys64/mingw64/include -isystem D:/msys64/opt/ngtcp2/include -isystem D:/msys64/opt/ngtcp2/include -isystem D:/msys64/opt/nghttp3/include
   LDFLAGS:         -Wl,-rpath,/opt/openssl/lib -LD:/msys64/mingw64/lib -LD:/msys64/mingw64/lib -LD:/msys64/opt/ngtcp2/lib -LD:/msys64/opt/ngtcp2/lib -LD:/msys64/opt/nghttp3/lib
   LIBS:            -lnghttp3 -lngtcp2_crypto_openssl -lngtcp2 -lbcrypt -ladvapi32 -lcrypt32 -lssl -lcrypto -lssl -lcrypto -lgdi32 -lwldap32 -lzstd -lz -lws2_32

  curl version:     7.85.0-DEV
  SSL:              enabled (OpenSSL)
  SSH:              no      (--with-{libssh,libssh2})
  zlib:             enabled
  brotli:           no      (--with-brotli)
  zstd:             enabled (libzstd)
  GSS-API:          no      (--with-gssapi)
  GSASL:            no      (libgsasl not found)
  TLS-SRP:          enabled
  resolver:         POSIX threaded
  IPv6:             enabled
  Unix sockets:     no      (--enable-unix-sockets)
  IDN:              no      (--with-{libidn2,winidn})
  Build libcurl:    Shared=yes, Static=yes
  Built-in manual:  no      (--enable-manual)
  --libcurl option: enabled (--disable-libcurl-option)
  Verbose errors:   enabled (--disable-verbose)
  Code coverage:    disabled
  SSPI:             no      (--enable-sspi)
  ca cert bundle:   no
  ca cert path:     no
  ca fallback:      no
  LDAP:             enabled (winldap)
  LDAPS:            enabled
  RTSP:             enabled
  RTMP:             no      (--with-librtmp)
  PSL:              no      (libpsl not found)
  Alt-svc:          enabled (--disable-alt-svc)
  Headers API:      enabled (--disable-headers-api)
  HSTS:             enabled (--disable-hsts)
  HTTP1:            enabled (internal)
  HTTP2:            no      (--with-nghttp2, --with-hyper)
  HTTP3:            enabled (ngtcp2 + nghttp3)
  ECH:              no      (--enable-ech)
  Protocols:        DICT FILE FTP FTPS GOPHER GOPHERS HTTP HTTPS IMAP IMAPS LDAP LDAPS MQTT POP3 POP3S RTSP SMB SMBS SMTP SMTPS TELNET TFTP
  Features:         AsynchDNS HSTS HTTP3 HTTPS-proxy IPv6 Largefile NTLM SSL TLS-SRP alt-svc libz threadsafe zstd

  WARNING:  HTTP3 enabled but marked EXPERIMENTAL. Use with caution!

It seems to be quite hard to get HTTP/3 support compiled for Windows 😞

@tatsuhiro-t
Copy link
Contributor Author

It is because compiler sees different installation of openssl when actually compiling curl code.

As I wrote earlier:

You might need to specify CPPFLAGS and LDFLAGS to prioritize the header file locations for ngtcp2/nghttp3/openssl libraries compiled for mingw.

You need to specify CPPFLAGS and LDFLAGS to the path of quictls to tell compiler to use the correct library header files.

@mback2k
Copy link
Member

mback2k commented Jul 24, 2022

Thanks, I finally got it to build using the following config options:
./configure 'CPPFLAGS=-isystem D:/msys64/opt/openssl/include/' LDFLAGS=-LD:/msys64/opt/openssl/lib64 --host=x86_64-w64-mingw32 --build=x86_64-w64-mingw32 --prefix=/mingw64 --enable-werror --with-openssl=/opt/openssl --with-nghttp3=/opt/nghttp3 --with-ngtcp2=/opt/ngtcp2

@mback2k mback2k self-assigned this Jul 24, 2022
@mback2k
Copy link
Member

mback2k commented Jul 24, 2022

I can finally confirm that I can reproduce the issue and am now starting to tackle a possible solution.

@mback2k
Copy link
Member

mback2k commented Jul 25, 2022

I need a test server that supports uploading maybe 100 MB using HTTP1/2 and 3 to time the impact of my fix. Any suggestions?

@mback2k
Copy link
Member

mback2k commented Jul 25, 2022

Right now I can only confirm the expected. The empty-send()-workaround is definitely needed to reset the state of FD_WRITE between multi_wait calls in case no other send() was done. If it is no in place then you can clearly see curl pausing on uploads every now and then in the console output. With the existing workaround in place, the upload is done smoothly using HTTP/1.1.

But unfortunately my upload attempts using HTTP/3 are never concluding with or without the workaround active. I just get an endless stream of ngh3_stream_recv returns 0 bytes and EAGAIN in the verbose output once the request headers are sent.

Any idea why this might be?

@tatsuhiro-t
Copy link
Contributor Author

Which server are you using for HTTP/3 upload? I use curl -T and it completes successfully.

@mback2k
Copy link
Member

mback2k commented Jul 26, 2022

I used curl -d against https://nghttp2.org:4433/httpbin/post.

@tatsuhiro-t
Copy link
Contributor Author

It looks like postfield is set, upload progress indicator goes to 100% instantly, but the actual upload is done much slower than that.
Lots of EAGAIN smells like a bug.

@tatsuhiro-t
Copy link
Contributor Author

tatsuhiro-t commented Jul 26, 2022

Right, so the lots of ngh3_stream_recv returns 0 bytes and EAGAIN is caused by IP_RECVERR socket option set for UDP socket. ngtcp2 does UDP Path MTU Discovery and it might send larger packet than the path can sustain. In this case ICMP message is received, it is resulted in POLLERR, which is translated into POLLIN. Because IP_RECVERR, the normal recvfrom call does not clear error queue, thus poll keeps returning POLLERR.

IP_RECVERR is set only for linux, so this probably is not directly related to mingw build. I'll file an another issue for this.

@mback2k
Copy link
Member

mback2k commented Jul 28, 2022

@tatsuhiro-t I get the same issue with -T file as with -d @file.

@mhils
Copy link

mhils commented Jan 12, 2023

Are there any cURL Windows build configurations that are not affected by this? We'd like to get Windows + HTTP/3 CI for mitmproxy going, but this is breaking my tests locally. :)

@bagder
Copy link
Member

bagder commented Jan 12, 2023

#9203 contains efforts of a fix

jay added a commit to jay/curl that referenced this issue Feb 7, 2023
- Limit the 0-sized send procedure that is used to reset a SOCKET's
  FD_WRITE to TCP sockets only.

Prior to this change the reset was used on UDP sockets as well, but
unlike TCP sockets a 0-sized send actually sends out out a datagram.

Assisted-by: Marc Hörsken

Ref: curl#9203

Fixes curl#9086
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Feb 12, 2023
- Limit the 0-sized send procedure that is used to reset a SOCKET's
  FD_WRITE to TCP sockets only.

Prior to this change the reset was used on UDP sockets as well, but
unlike TCP sockets a 0-sized send actually sends out out a datagram.

Assisted-by: Marc Hörsken

Ref: curl#9203

Fixes curl#9086
Closes #xxxx
@jay jay closed this as completed in f438ce0 Feb 13, 2023
bch pushed a commit to bch/curl that referenced this issue Jul 19, 2023
- Limit the 0-sized send procedure that is used to reset a SOCKET's
  FD_WRITE to TCP sockets only.

Prior to this change the reset was used on UDP sockets as well, but
unlike TCP sockets a 0-sized send actually sends out a datagram.

Assisted-by: Marc Hörsken

Ref: curl#9203

Fixes curl#9086
Closes curl#10430
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HTTP/3 h3 or quic related Windows Windows-specific
6 participants