multi: improve wakeup and wait code

[icing]

• Split WINSOCK and POSIX code in multi_wait() as the ifdef'ery was becoming unreadable
• define ENABLE_WAKEUP to mean the wakeup socketpair is enabled, no additional USE_WINSOCK check needed. Under WINSOCK ENABLE_WAKEUP is not defined, so it's availability is as before under the double defined() checks
• When the multi handle has "alive" transfers, the admin handle's pollset include the wakeup receive socket. This results in the admin handle running when someone uses curl_multi_wakeup().
• Without any "alive" transfers, the wakeup socket is removed from the pollset. Otherwise, event based processing would never finish, eg. leave the event loop.
• The wakeup socket was never registered for event processing before, e.g. curl_multi_wakeup() never worked in that mode.
• Adjust test expectations on socket callback invocations and number of sockets appearing in waitfds sets.

[icing]

@jay please have a look if my split of the WINSOCK code looks sane.

[jay]

@icing sorry I did not see this until it was bumped in my inbox. @mback2k wrote the Winsock select code. I don't grok this area but I took a look.

I notice that your split logic change removes multi_wait's extrawait parameter ("when no socket, wait") and replaces it with wait_on_nop which afaict does the same thing. However the logic has also changed so that the wakeup sockets are no longer polled unless wait_on_nop is true, whereas before they were polled if (eliminated parameter) use_wakeup was true. Shouldn't they always be polled if wakeup is enabled, regardless of if there's an extra wait, or should that be the extra wait (ie waiting on the wakeup sockets when there's nothing else to wait on)?

Also, the extra wait handling has been moved into the split-off functions and is handled differently depending on winsock or posix (ie non-winsock). For the winsock version it doesn't call multi_timeout to calculate the extra wait time any longer if there are no sockets. What I'm saying is before it was this:

multi_wait does a unix poll or winsock poll
if no fds (afaict this means no wakeup sockets either) and extrawait is true then multi_timeout is called to get the wait time and wait that time

now it's either
multi_wait calls multi_posix_poll
multi_posix_poll when no fds calls multi_timeout to calculate the extra wait time and waits if necessary

multi_wait calls multi_winsock_select (Windows)
multi_winsock_select does not do this

It is not entirely clear to me whether the winsock function that was split off is correct, but again I don't grok this area, maybe Marc can take a look.

[mback2k]

This area was always very complicated and I remember spending a lot of time even getting into this initially.
Therefore, it is very hard to follow this refactor considering the amount of changes per individual commit.

The difference described by @jay may actually be quite important, because I remember that I spent a lot of time to make the wake-able timeout code work correctly on Windows using WSAEventSelect and WSAWaitForMultipleEvents. Especially the use of WSAEventSelect was quite tricky as noted in the various inline code comments.

[icing]

Thanks for reviewing and your comments. I agree that the code has been very complicated and we seem to have no adequate test cases (@mback2k how did you verify your changes in the past?).

My reason for splitting the windows and posix cases are twofold:

1. the nested #ifdefs made it hard to read.
2. This PR prepares use of the wakeup socketpair for the notification mechanism in async DNS resolves. if ENABLE_WAKEUP is defined, it needs to work,
3. ENABLE_WAKEUP was overridden by USE_WINSOCK in several places. It seemed to me it was no t possible to use the wakeup socketpair on Windows, even though it was defined.

The former code in multi.c:1435 did:

#ifdef ENABLE_WAKEUP
#ifndef USE_WINSOCK
  if(use_wakeup && multi->wakeup_pair[0] != CURL_SOCKET_BAD) {
    if(Curl_pollfds_add_sock(&cpfds, multi->wakeup_pair[0], POLLIN)) {
      mresult = CURLM_OUT_OF_MEMORY;
      goto out;
    }
  }
#endif
#endif

So the wakeup socket was never part of the pollset under Windows. Something needed to be done here.

As to @jay's observations:

1. the former parameter use_wakeup was always the same as extrawait, so I just eliminated it. This is not really what I wanted. We now always want to add the wakeup socket to the pollset when it is available. I will make an additional PR that changes this.
2. the former parameter extrawait I renamed to wait_on_nop. Which is not the greatest name in history either. Names...
3. The difference in handling of multi_timeout is indeed wrong. The correct fix seems to be to remove it from the posix variant. As you can see, multi_timeout() is already called before calling the windows/posix functions. There is no need to call it again.

I'll make a follow-up PR with these changes.

[icing]

I made #21072 to fix the things discovered in review. Thanks!

[jay]

I hate to say it but if we can't be sure the Winsock event code is going to do what is expected, or we are not sure it is correct for this forthcoming remodel, then we should consider dropping it in favor of the posix code which also works on Windows. IIRC the Winsock specific event style of poll has an advantage in that it allows more than 1024 sockets (edit: looks like the advantage would be more than 64 sockets since that's the default that select allows, unless FD_SETSIZE is redefined like to 32767), and possibly other advantages I can't remember atm. It has a disadvantage in that sockets need to have FD_WRITE explicitly reset, which can only be done for TCP sockets, see function reset_socket_fdwrite. (I'm not sure of potential issues with UDP sockets and HTTP/3 since those sockets can't have FD_WRITE reset. In practice that probably isn't an issue though.)

tested ok with it disabled, shows similar numbers of CPU cycles in repeated runs of 200MB download curld http://cachefly.cachefly.net/200mb.test -o NUL

diff --git a/lib/multi.c b/lib/multi.c
index 7646e12..3911ed6 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -1330,7 +1330,7 @@ CURLMcode curl_multi_waitfds(CURLM *m,
   return mresult;
 }
 
-#ifdef USE_WINSOCK
+#if 0
 /* Reset FD_WRITE for TCP sockets. Nothing is actually sent. UDP sockets cannot
  * be reset this way because an empty datagram would be sent. #9203
  *
@@ -1610,7 +1610,7 @@ static CURLMcode multi_wait(struct Curl_multi *multi,
     CURL_TRC_M(data, "multi_wait(fds=%d, timeout=%d) tinternal=%ld",
                cpfds.n, timeout_ms, timeout_internal);
 
-#ifdef USE_WINSOCK
+#if 0
   mresult = multi_winsock_select(multi, &cpfds, curl_nfds,
                                  extra_fds, extra_nfds,
                                  timeout_ms, extrawait, &nevents);

That said I don't have any objections to #21072

[icing]

I am all for simplifying this, but I cannot judge the impacts this change would have.

[mback2k]

The POSIX code does not work with stdin/stdout on Windows as far as I remember, so piping with curl.exe was problematic with Windows Event style handling.

I would have to trace the origins of why I introduced this at the time which is not so easy to many refactors and me currently traveling on vacation.

[jay]

I would have to trace the origins of why I introduced this at the time which is not so easy to many refactors and me currently traveling on vacation.

Understood. My disabled test was quick and dirty so yes there may be some disadvantages. It doesn't address FD_SETSIZE issue (too large FD_SETSIZE with fd_set declarations on the stack could be a problem) or the wakeup code.

Coming back to what Stefan said earlier, if Winsock is used then ENABLE_WAKEUP is not defined (both before and after this pr). This is because instead of a socketpair to wake up the multi, the winsock builds use a wakeup from an event handle in the multi wsa_event. So if we drop multi_winsock_select for Windows then we'd have to switch back to using a socketpair for the wakeup (ie define ENABLE_WAKEUP on Windows).

Currently it's one or the other:

from 8.19.0:

curl/lib/multihandle.h

Lines 161 to 169 in 8c908d2

	#ifdef USE_WINSOCK
	WSAEVENT wsa_event; /* Winsock event used for waits */
	#else
	#ifdef ENABLE_WAKEUP
	curl_socket_t wakeup_pair[2]; /* eventfd()/pipe()/socketpair() used for
	wakeup 0 is used for read, 1 is used
	for write */
	#endif
	#endif

from master (has the changes from this pr):

curl/lib/multihandle.h

Lines 74 to 76 in 797bc31

	#if !defined(CURL_DISABLE_SOCKETPAIR) && !defined(USE_WINSOCK)
	#define ENABLE_WAKEUP
	#endif

curl/lib/multihandle.h

Lines 161 to 168 in 797bc31

	#ifdef USE_WINSOCK
	WSAEVENT wsa_event; /* Winsock event used for waits */
	#endif
	#ifdef ENABLE_WAKEUP
	curl_socket_t wakeup_pair[2]; /* eventfd()/pipe()/socketpair() used for
	wakeup 0 is used for read, 1 is used
	for write */
	#endif

I think though this would explain what Stefan said about observing that with winsock a wakeup socket is never part of the pollset because it wouldn't be since an event is used to wake up instead. As seen in curl_multi_wakeup.

[mback2k]

Understood. I was able to find the original PR which also links a bug as reason for the event-based implementation: #6245

There were also some follow-up PRs with further fixes, enhancements and finally additional enabled tests which didn't work before.

I think MinGW and other implementations of POSIX derived from msys/cygwin had some problems without this as well.

It was a tedious process to get this correct initially on all platforms. I am worried the refactor won't be tested as thoroughly since the Azure and AppVeyor tests regarding MinGW have been dropped as far as I understand.