@rcombs @jay @bagder please take a look at my first attempt to fix/improve #6146 based upon #6146 (comment).

Seems like this currently breaks test 1564. I will work on this (hopefully) this evening.

@tpodom would you mind helping test the performance with this PR, related to #6146?

@mback2k sorry for the delay getting back to you. The timeout delay still occurs. I see the pollrc = 0 but then the WSAWaitForMultipleEvents delays for 1000 ms.

I did a bit more playing around and I understand why I'm not hitting the WSAEWOULDBLOCK error. The select checks either in the multi-wait or the socket check prior to send that the socket is ready and if not, the send is not attempted. Since the send is never attempted to trigger WSAEWOULDBLOCK, the FD_WRITE is never signaled for the event so the WSAWaitForMultipleEvents just delays.

I played around with moving the polling after the transfer call and I do see a couple of times where the would block is hit on the send if I modify the send to ignore the socket check but then the timeout_ms delay kicks in and each iteration that gives it enough time to clear up to no longer block.

I think what would really be needed is to set some flag when the WSAEWOUDLBLOCK occurs and then check if that's been set before calling WSAWaitForMultipleEvents. That would require ignoring the socket readiness before send though in transfer.c:

  /* If we still have writing to do, we check if we have a writable socket. */
  if ((k->keepon & KEEP_SEND) && (select_res & CURL_CSELECT_OUT)) {

Basically, try to replicate the logic as described by MS:

"Therefore, an application can assume that sends are possible starting from the first FD_WRITE network event setting and lasting until a send returns WSAEWOULDBLOCK. After such a failure the application will find out that sends are again possible when an FD_WRITE network event is recorded and the associated event object is set."

Basically, try to replicate the logic as described by MS:

"Therefore, an application can assume that sends are possible starting from the first FD_WRITE network event setting and lasting until a send returns WSAEWOULDBLOCK. After such a failure the application will find out that sends are again possible when an FD_WRITE network event is recorded and the associated event object is set."

Makes sense. So an actual fix would require general behavior changes in curl outside of multi.c. One question would be: could this be the behavior regardless of platform or would we need to adjust the behavior just on Windows (with WinSock)? cc @bagder @jay

I was thinking about this a bit more and what I'm thinking may not really work at all since I don't have a good grasp on how all of this fits together but what about something along the following:

Curl_send_plain returns CURLE_AGAIN which is handled in Curl_write. Instead of just returning 0, what if Curl_write updates the connection to indicate the last write failed with CURLE_AGAIN.

Then, in Curl_multi_wait, only perform the WSAWaitForMultipleEvents if there is no data to write (can we know that in Curl_multi_wait?) or if the write socket hit CURLE_AGAIN. Because if we have data to write and have not hit CURLE_AGAIN then Windows says we should assume we can keep writing.

Then, for the socket check in Curl_readwrite, could Curl_multi_wait set the conn->cselect_bits so the check is skipped, or could Curl_poll be updated to skip the select unless we previously hit CURLE_AGAIN?

@tpodom thanks for the input. I may have found an easier way to reset FD_WRITE by just sending zero bytes before trying to wait on a socket for write readiness again. Would you mind testing the new implementation in this updated PR? cc @jay @rcombs

Using the same test from #6146:

master (796c068 2020-12-14)
Transfer rate: 3992 KB/sec (52429310 bytes in 13 seconds)
Transfer rate: 4124 KB/sec (52429310 bytes in 12 seconds)
Transfer rate: 4148 KB/sec (52429310 bytes in 12 seconds)

mback2k:multi-consolidate-syscalls (c502f6a 2020-12-15)
Transfer rate: 4107 KB/sec (52429310 bytes in 12 seconds)
Transfer rate: 4059 KB/sec (52429310 bytes in 13 seconds)
Transfer rate: 4141 KB/sec (52429310 bytes in 12 seconds)

@mback2k sorry for the delay getting back to you. The latest change looks to be working great for me. I tested with auto tuning both enabled and disabled on the server and did not experience any significant delays and the time to upload was consistent. Good find on using the 0 byte send to trigger the FD_WRITE!

@tpodom no problem, thanks for testing. @jay thanks for verifying the performance, would you mind reviewing the PR?

Can I also get you @Togtja, @ngg, @rmja, @tmkk, @RadMission to test this PR regarding up- and download performance issues you experienced with past iterations?

Using the same postupload example to the closest test server (ny2 for me) the results are very similar.

master (f83d4ea 2021-03-14)
Transfer rate: 4123 KB/sec (52429310 bytes in 12 seconds)
Transfer rate: 3984 KB/sec (52429310 bytes in 13 seconds)
Transfer rate: 4043 KB/sec (52429310 bytes in 13 seconds)
Transfer rate: 4101 KB/sec (52429310 bytes in 12 seconds)
Transfer rate: 4206 KB/sec (52429310 bytes in 12 seconds)
Transfer rate: 3951 KB/sec (52429310 bytes in 13 seconds)

mback2k:multi-consolidate-syscalls (ca6ce77 2021-03-12)
Transfer rate: 4013 KB/sec (52429310 bytes in 13 seconds)
Transfer rate: 4045 KB/sec (52429310 bytes in 13 seconds)
Transfer rate: 4047 KB/sec (52429310 bytes in 13 seconds)
Transfer rate: 4030 KB/sec (52429310 bytes in 13 seconds)
Transfer rate: 4079 KB/sec (52429310 bytes in 13 seconds)
Transfer rate: 4018 KB/sec (52429310 bytes in 13 seconds)

master (what multi-consolidate-syscall is currently branched off of) is overall a few KB faster but that is not of significance due to network conditions.

@jay thanks a lot for testing this again! I would really appreciate your approval on this PR then if you feel like it is ready to go in during the next feature window. I will also perform some additional upload and download performance tests before merge.

Rebased to current master in order to get ready for merge during this feature window.

Rebased to current master in order to get ready for merge during this feature window.

bugfixes only until the patch release on the 14th

Rebased again to current master in order to get ready for merge during this feature window.

Rebased again due to previously failing test 1660 which was fixed with 6b97f13.

Thanks everyone! @rcombs @tpodom @tmkk @jay @MarcelRaad