Fix for HTTP/3: Windows build sends lots of empty UDP datagrams

[mback2k]

Attempt to #9086 by: fix sending empty UDP packets to reset FD_WRITE

On Windows the internal state of FD_WRITE as returned from
WSAEnumNetworkEvents is only reset after successful send().
Since multi_wait can be called multiple times without such
a send() happening in between calls, we need to call send()
ourselves to make sure we still get a fresh FD_WRITE signal.

For TCP connections an empty send() does not actually send
something on the wire, but for UDP an empty message packet
is actually send on the wire, leading to hundreds of such.

This commit makes the send()-workaround apply only to TCP.

[mback2k]

@tatsuhiro-t hey, can you please test drive this workaround for me and compare the performance before and after this PR?

[tatsuhiro-t]

I saw almost no difference in between. I also saw no difference when I tested HTTP/1.1 upload with 0 bytes send() commented out, so I might not have a good environment to test this thing.

[mback2k]

The test upload should be at least something around 100 MB to notice a difference.

[tatsuhiro-t]

I actually used 100MiB file.

[bagder]

Does this calling of getsockopt() on all the sockets risk slowing things down?

It struck me that as we already have a "socket hash table" with all the sockets in use within the multi handle, it could easily be extended to have the socket type in there as well. It could then make it a hash table lookup instead, if this getsockopt() approach turns out unfavorable.

[mback2k]

Does this calling of getsockopt() on all the sockets risk slowing things down?

I would think so, yes. But unfortunately, it is currently quite hard to test with the state of uploads using HTTP/3.

It struck me that as we already have a "socket hash table" with all the sockets in use within the multi handle, it could easily be extended to have the socket type in there as well. It could then make it a hash table lookup instead, if this getsockopt() approach turns out unfavorable.

Good idea! Would you mind taking a look into this or pointing me into the right direction? Where would the hash table value be initialized and stored?

[bagder]

The function multi.c:sh_getentry() returns a struct Curl_sh_entry from the socket hash given a socket that is used by libcurl. That struct could probably be extended to also contain sock type, maybe even lazily initialized so you could do the getsockopt() in case it doesn't know and then store the result in case you need to know again for this socket later.

[mback2k]

@bagder I attempted to implement the caching, but with the code committed in 7ef77d2 including debug fprintf statements, I always get entry=0, so the checked socket is not already in the hashmap. Should I add it in that case?

Also I observed that multi_wait is only used to check a socket for write readiness with the HTTP/3 protocol up until the request headers are sent. After that, I no longer get any entry=0 line, but just * ngh3_stream_recv returns 0 bytes and EAGAIN lines.

For TFTP and pre-HTTP/3 uploads, I get entry=0 lines throughout the transfer as somewhat expected. @tatsuhiro-t

[mback2k]

@bagder @tatsuhiro-t I could really use your help and insight on this topic. Thanks in advance!

[mback2k]

Force-pushed with a new commit adding the socket to the hash table if it wasn't there yet:

curl/lib/multi.c

Lines 1170 to 1171 in edf8ef9

	if(!entry)
	entry = sh_addentry(&multi->sockhash, s);

Let's see what CI will tell us if this is the right approach...

[mback2k]

CI failures are unrelated to the socket hash table. But @bagder , I still have doubts about adding sockets from extra_fds to that hash table belonging to a multi handle. Since we are not managing the extra_fds within libcurl, but the calling application does, those socket handles/numbers can change over time and eventually get re-used by a connection of a different type. So we have no way to invalidate our cache for these sockets and may never notice the changed socket's connection type. This means we probably should only cache the socket's connection type for our sockets, right?

[mback2k]

@tatsuhiro-t although this still contains debug code, can you review the impact of this PR again please?

[tatsuhiro-t]

It works for me. No empty packets were observed.

[mback2k]

@tatsuhiro-t thanks a lot! @bagder does the logic in this PR make sense to you now?

[mback2k]

I am still not sure if using sh_addentry this way is correct, but I am now requesting review on this PR in general.

[jay]

I am still not sure if using sh_addentry this way is correct

I share your concerns. I would change the extra fd part to call getsockopt directly. It is not ideal. I walked through it and there's a number of interlocked calls and a critical section lock to retrieve the socket information, but that may be true of send as well. You could use tvnow (which calls QueryPerformanceCounter) then make a bunch of sockets and call multi_wait 1000x to compare.

The regular fd part if I recall the sockets should already have entries, and if they don't I can't readily tell if it's ok to create one without setting the rest of the entry as would be done in singlesocket. So, perhaps getsockopt should be added in singlesocket here and then multi_getsocktype return the type but not set it lazily

[mback2k]

The regular fd part if I recall the sockets should already have entries, and if they don't I can't readily tell if it's ok to create one without setting the rest of the entry as would be done in singlesocket. So, perhaps getsockopt should be added in singlesocket here and then multi_getsocktype return the type but not set it lazily

In all my tests the entry wasn't existing for the socket in the regular fd part at the time multi_getsocktype was called. So I guess then I just probably just stay with always calling getsockopt without a caching layer.

[jay]

Interesting. That's not what I expected. In that case I'd wait for @bagder's input on this.

[bagder]

The socket gets added to the sockhash on:

curl/lib/multi.c

Line 2879 in aa970c4

entry = sh_addentry(&multi->sockhash, s);

I have not analyzed why the sockets you check are not present in that hash. It seems curious to me.

[mback2k]

Thanks to both of you. I tried to incorporate your feedback, @jay.

[jay]

I think the function could use a comment that it is only to be called on libcurl sockets and not the extra sockets, and also explain why it defaults to SOCK_STREAM when no entry is available or the socket type cannot be obtained. Conversely the second check sets the type to 0 when it cannot be obtained

[mback2k]

I think this function can be called for extra sockets as they may overlap with libcurl sockets. We cannot assume that they are fully distinct or can we?

[jay]

I encourage you to test your caching method against getsockopt on 1000 sockets so we can see if it has any performance gain and is worth pursuing. For example

  if(getsockopt(s, SOL_SOCKET, SO_TYPE, (char *)&st, &sl) != SOCKET_ERROR &&
     st == SOCK_STREAM) {
    send(s, NULL, 0, 0); /* reset FD_WRITE */
  }

[icing]

I would propose to beef up the getsock() interfaces to collect not only the sockfd and read/write flags, but also the type. The code which opened the socket does know what type it is. So, there is really no need to ask for that in multi_wait().

Since this is not done in a day, I think we could merge this PR and then do the refactoring afterwards, eliminating multi_getsocktype() again.

[mback2k]

Unfortunately, curl does not open the sockets in extra_fds. But yes, I hope to get to this again over the holidays period. Currently to busy at work, sorry.

[mback2k]

Still too busy with life and work, sorry. Maybe @icing or @jay could take over from here?

My curl time will be very limited till much later this year, sorry.

[jay]

See #10430 for my take on this, which is calling getsockopt directly without caching, and then reset FD_WRITE only if TCP. It's unclear to me if extra user fd's that could possibly? be neither TCP nor UDP would need to reset FD_WRITE.