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

multi: implement wait using winsock events #5634

Closed
wants to merge 1 commit into from

Conversation

@rcombs
Copy link
Contributor

rcombs commented Jun 30, 2020

Repeat of #5397, but this time with #5631 fixed. In theory, anyway; I haven't had a chance to test this out on a Windows machine myself yet. Also not sure how to add a test (particularly one for something that takes longer than it should, but otherwise finishes correctly).

@rcombs rcombs force-pushed the rcombs:winsock-redux branch from 9981a3f to 756c553 Jun 30, 2020
@mback2k
Copy link
Member

mback2k commented Jun 30, 2020

Thanks, I will take a look later this week. This reminds me of a similar issue and solution I had with sockfilt.c.

lib/multi.c Outdated Show resolved Hide resolved
lib/multi.c Outdated Show resolved Hide resolved
@rcombs rcombs force-pushed the rcombs:winsock-redux branch 3 times, most recently from f62a6dc to 8805a4b Jul 1, 2020
@mback2k
mback2k approved these changes Jul 5, 2020
Copy link
Member

mback2k left a comment

LGTM

@mback2k mback2k requested review from MarcelRaad and jay Jul 5, 2020
@mback2k
Copy link
Member

mback2k commented Jul 5, 2020

One thing which we missed last time: This makes curl itself depend on WinSock2 for the first time. Currently it is theoretically possible to still run curl on versions older than Windows 95 (NT 3.5), but with this change curl requires at least Windows 98 (NT 4.0) or Windows 95 (NT 3.5) with the WinSocks2 addon package. I don't know if these historic Windows versions still matter to us.

Copy link
Member

MarcelRaad left a comment

LGTM. I don't think requiring WinSock 2 is a problem as we require the February 2003 Platform SDK anyway, which doesn't support anything older than Windows 95, and Windows NT before 3.5 didn't support WinSock at all.

@mback2k
Copy link
Member

mback2k commented Jul 6, 2020

LGTM. I don't think requiring WinSock 2 is a problem as we require the February 2003 Platform SDK anyway, which doesn't support anything older than Windows 95, and Windows NT before 3.5 didn't support WinSock at all.

Side note: this means we can also get rid of some LoadLibrary calls in telnet.c and just call WinSock 2 directly:

curl/lib/telnet.c

Lines 1343 to 1392 in 0f55269

/*
** This functionality only works with WinSock >= 2.0. So,
** make sure we have it.
*/
result = check_wsock2(data);
if(result)
return result;
/* OK, so we have WinSock 2.0. We need to dynamically */
/* load ws2_32.dll and get the function pointers we need. */
wsock2 = Curl_load_library(TEXT("WS2_32.DLL"));
if(wsock2 == NULL) {
failf(data, "failed to load WS2_32.DLL (%u)", GetLastError());
return CURLE_FAILED_INIT;
}
/* Grab a pointer to WSACreateEvent */
create_event_func =
CURLX_FUNCTION_CAST(WSOCK2_EVENT,
(GetProcAddress(wsock2, "WSACreateEvent")));
if(create_event_func == NULL) {
failf(data, "failed to find WSACreateEvent function (%u)", GetLastError());
FreeLibrary(wsock2);
return CURLE_FAILED_INIT;
}
/* And WSACloseEvent */
close_event_func = GetProcAddress(wsock2, "WSACloseEvent");
if(close_event_func == NULL) {
failf(data, "failed to find WSACloseEvent function (%u)", GetLastError());
FreeLibrary(wsock2);
return CURLE_FAILED_INIT;
}
/* And WSAEventSelect */
event_select_func = GetProcAddress(wsock2, "WSAEventSelect");
if(event_select_func == NULL) {
failf(data, "failed to find WSAEventSelect function (%u)", GetLastError());
FreeLibrary(wsock2);
return CURLE_FAILED_INIT;
}
/* And WSAEnumNetworkEvents */
enum_netevents_func = GetProcAddress(wsock2, "WSAEnumNetworkEvents");
if(enum_netevents_func == NULL) {
failf(data, "failed to find WSAEnumNetworkEvents function (%u)",
GetLastError());
FreeLibrary(wsock2);
return CURLE_FAILED_INIT;
}

@jay jay added the Windows label Jul 6, 2020
@ngg
Copy link
Contributor

ngg commented Jul 10, 2020

I tried to reproduce the slow sftp upload issue at #5633
I managed to reproduce it, but only with 7.71.0. For me it was working good when I tried with 7.71.1 (in contrast to what @rmja experienced there)
I also tried with this PR applied and it was still working good.

@mback2k mback2k requested a review from bagder Jul 14, 2020
@mback2k
Copy link
Member

mback2k commented Jul 14, 2020

@bagder Do you approve giving this another try for this release?

@jay
Copy link
Member

jay commented Jul 15, 2020

If we can't reproduce what @rmja described I'd say go for it.

@bagder
Copy link
Member

bagder commented Jul 15, 2020

I agree with @jay. It would be good to get a test case added that would reproduce the previous bug and see that it no longer does...

@mback2k
Copy link
Member

mback2k commented Jul 15, 2020

I will try to turn the example given by @rmja into a test case, to make sure we catch this error in the future.

@rmja
Copy link

rmja commented Jul 15, 2020

Let me know if you can produce a prerelease build, then I can test to see if it works or not.

@mback2k
Copy link
Member

mback2k commented Jul 21, 2020

Regarding the test case I wanted to create: we already have many FTP and SFTP upload test cases in our testsuite, so the use case should already be covered. The only problem is: we are currently not running SCP/SFTP tests on any Windows CI since I migrated my builds from buildbot to Azure Pipelines. I lost libssh2 support on the way and it is still on my ToDo list to bring that back.

In the mean time I would like to ask @rmja and @RadMission to check if this PR still has the problem for them. The Windows CI jobs for this PR successfully ran all the relevant HTTP and FTP upload tests at least, so if there is still any problem, I guess it is with SCP/SFTP. Please help us test this PR manually for now. Thanks in advance!

@rmja
Copy link

rmja commented Jul 21, 2020

I will be happy to test this on Windows if you can send a link to a downloadable build.

@mback2k
Copy link
Member

mback2k commented Jul 21, 2020

Okay, I can probably provide a debug build tomorrow.

@mback2k
Copy link
Member

mback2k commented Jul 22, 2020

@rmja attached you will find a debug build created with mingw-w64 from this PR on current curl and libssh2 master.

I was successfully able to do an SFTP upload with this build: curl.exe -k sftp://test:test@127.0.0.1/ --upload-file curl.exe

@rmja
Copy link

rmja commented Jul 23, 2020

@mback2k My test suite fails with this build - I get SSL_CONNECT_ERROR when I do easy_perform. The curl builds that I have used until now has openssl and libcrypto as dependencies. I have of cause extracted all the files in the zip you provided, but is there any other dependency that I need to include for this build to work correctly?

@mback2k
Copy link
Member

mback2k commented Jul 23, 2020

@rmja that build is just using libssh2 (with Windows-native WinCNG crypto backend) and curl (with Windows-native Schannel TLS backend). So it should work on recent Windows (>= Vista) without any additional dependencies except this mingw-w64 libwinpthread library. What is your exact test case?

@rmja
Copy link

rmja commented Jul 23, 2020

Here is the outline of handle configuration. I have to proxy as the target server is IP filtered.

var handle = SafeEasyHandle.Init();

CurlNative.Easy.SetOpt(handle, CURLoption.USERNAME, credentials.UserName);
CurlNative.Easy.SetOpt(handle, CURLoption.PASSWORD, credentials.Password);
CurlNative.Easy.SetOpt(handle, CURLoption.PROXYTYPE, CurlProxyType.Socks5);
CurlNative.Easy.SetOpt(handle, CURLoption.PROXY, proxy.Host);
CurlNative.Easy.SetOpt(handle, CURLoption.PROXYPORT, proxy.Port);
CurlNative.Easy.SetOpt(handle, CURLoption.PROXYUSERPWD, proxy.UserInfo);
CurlNative.Easy.SetOpt(handle, CURLoption.SSL_VERIFYPEER, 0);
CurlNative.Easy.SetOpt(handle, CURLoption.URL, url.ToString());
CurlNative.Easy.SetOpt(handle, CURLoption.USE_SSL, CurlUseSsl.All);
CurlNative.Easy.SetOpt(handle, CURLoption.CUSTOMREQUEST, "MLSD");
CurlNative.Easy.SetOpt(handle, CURLoption.WRITEFUNCTION, (data, size, nmemb, user) =>
{
    // Handler is not called
});

var result = CurlNative.Easy.Perform(handle);
result == SSL_CONNECT_ERROR
@mback2k
Copy link
Member

mback2k commented Jul 26, 2020

Okay, I modified the socket readyness pre-check handling for this build. Please try it.

From 69c4f15179148254e1c02fbe4764a210d15c038b Mon Sep 17 00:00:00 2001
From: Marc Hoersken <info@marc-hoersken.de>
Date: Sun, 26 Jul 2020 21:26:46 +0200
Subject: [PATCH] multi: pre-check readyness of all sockets before waiting

---
 lib/multi.c | 72 +++++++++++++++++++++++++++++--------------------------------
 1 file changed, 34 insertions(+), 38 deletions(-)

diff --git a/lib/multi.c b/lib/multi.c
index afd36e4..b655d1a 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -1080,7 +1080,7 @@ static CURLMcode Curl_multi_wait(struct Curl_multi *multi,
   struct pollfd *ufds = &a_few_on_stack[0];
   bool ufds_malloc = FALSE;
 #else
-  int already_writable = 0;
+  struct pollfd pre_poll;
   DEBUGASSERT(multi->wsa_event != WSA_INVALID_EVENT);
 #endif
 
@@ -1161,13 +1161,15 @@ static CURLMcode Curl_multi_wait(struct Curl_multi *multi,
     while(data) {
       bitmap = multi_getsock(data, sockbunch);
 
-      for(i = 0; i< MAX_SOCKSPEREASYHANDLE; i++) {
+      for(i = 0; i < MAX_SOCKSPEREASYHANDLE; i++) {
         curl_socket_t s = CURL_SOCKET_BAD;
 #ifdef USE_WINSOCK
         long mask = 0;
 #endif
         if(bitmap & GETSOCK_READSOCK(i)) {
 #ifdef USE_WINSOCK
+          if(SOCKET_READABLE(sockbunch[i], 0) > 0)
+            timeout_ms = 0;
           mask |= FD_READ;
 #else
           ufds[nfds].fd = sockbunch[i];
@@ -1178,15 +1180,8 @@ static CURLMcode Curl_multi_wait(struct Curl_multi *multi,
         }
         if(bitmap & GETSOCK_WRITESOCK(i)) {
 #ifdef USE_WINSOCK
-          struct timeval timeout;
-          fd_set writefds;
-          timeout.tv_sec = 0;
-          timeout.tv_usec = 0;
-          FD_ZERO(&writefds);
-          FD_SET(sockbunch[i], &writefds);
-          if(select((int)sockbunch[i] + 1, NULL, &writefds, NULL,
-                    &timeout) == 1)
-            already_writable++;
+          if(SOCKET_WRITABLE(sockbunch[i], 0) > 0)
+            timeout_ms = 0;
           mask |= FD_WRITE;
 #else
           ufds[nfds].fd = sockbunch[i];
@@ -1213,23 +1208,30 @@ static CURLMcode Curl_multi_wait(struct Curl_multi *multi,
 #ifdef USE_WINSOCK
     long events = 0;
     extra_fds[i].revents = 0;
-    if(extra_fds[i].events & CURL_WAIT_POLLIN)
+    pre_poll.fd = extra_fds[i].fd;
+    pre_poll.events = 0;
+    pre_poll.revents = 0;
+    if(extra_fds[i].events & CURL_WAIT_POLLIN) {
       events |= FD_READ;
-    if(extra_fds[i].events & CURL_WAIT_POLLPRI)
+      pre_poll.events |= POLLIN;
+    }
+    if(extra_fds[i].events & CURL_WAIT_POLLPRI) {
       events |= FD_OOB;
+      pre_poll.events |= POLLPRI;
+    }
     if(extra_fds[i].events & CURL_WAIT_POLLOUT) {
-      struct timeval timeout;
-      fd_set writefds;
-      timeout.tv_sec = 0;
-      timeout.tv_usec = 0;
-      FD_ZERO(&writefds);
-      FD_SET(extra_fds[i].fd, &writefds);
-      if(select((int)extra_fds[i].fd + 1, NULL, &writefds, NULL,
-                &timeout) == 1) {
-        extra_fds[i].revents = CURL_WAIT_POLLOUT;
-        already_writable++;
-      }
       events |= FD_WRITE;
+      pre_poll.events |= POLLOUT;
+    }
+    if(Curl_poll(&pre_poll, 1, 0) > 0) {
+      if(pre_poll.revents & POLLIN)
+        extra_fds[i].revents |= CURL_WAIT_POLLIN;
+      if(pre_poll.revents & POLLOUT)
+        extra_fds[i].revents |= CURL_WAIT_POLLOUT;
+      if(pre_poll.revents & POLLPRI)
+        extra_fds[i].revents |= CURL_WAIT_POLLPRI;
+      if(extra_fds[i].revents)
+        timeout_ms = 0;
     }
     if(WSAEventSelect(extra_fds[i].fd, multi->wsa_event, events) != 0)
       return CURLM_INTERNAL_ERROR;
@@ -1259,8 +1261,6 @@ static CURLMcode Curl_multi_wait(struct Curl_multi *multi,
   if(nfds) {
     /* wait... */
 #ifdef USE_WINSOCK
-    if(already_writable > 0)
-      timeout_ms = 0;
     WSAWaitForMultipleEvents(1, &multi->wsa_event, FALSE, timeout_ms, FALSE);
 #else
     int pollrc = Curl_poll(ufds, nfds, timeout_ms);
@@ -1292,7 +1292,7 @@ static CURLMcode Curl_multi_wait(struct Curl_multi *multi,
           if(events.lNetworkEvents & FD_OOB)
             mask |= CURL_WAIT_POLLPRI;
 
-          if(events.lNetworkEvents != 0)
+          if(ret && events.lNetworkEvents != 0)
             retcode++;
         }
         WSAEventSelect(extra_fds[i].fd, multi->wsa_event, 0);
@@ -1323,19 +1323,15 @@ static CURLMcode Curl_multi_wait(struct Curl_multi *multi,
               WSANETWORKEVENTS events = {0};
               if(WSAEnumNetworkEvents(sockbunch[i], multi->wsa_event,
                                       &events) == 0) {
-                if(events.lNetworkEvents != 0)
+                if(ret && events.lNetworkEvents != 0)
                   retcode++;
               }
-              if(ret && !events.lNetworkEvents &&
-                 (bitmap & GETSOCK_WRITESOCK(i))) {
-                struct timeval timeout;
-                fd_set writefds;
-                timeout.tv_sec = 0;
-                timeout.tv_usec = 0;
-                FD_ZERO(&writefds);
-                FD_SET(sockbunch[i], &writefds);
-                if(select((int)sockbunch[i] + 1, NULL, &writefds, NULL,
-                          &timeout) == 1)
+              if(ret && !timeout_ms && !events.lNetworkEvents) {
+                if((bitmap & GETSOCK_READSOCK(i)) &&
+                   SOCKET_READABLE(sockbunch[i], 0) > 0)
+                  retcode++;
+                else if((bitmap & GETSOCK_WRITESOCK(i)) &&
+                   SOCKET_WRITABLE(sockbunch[i], 0) > 0)
                   retcode++;
               }
               WSAEventSelect(sockbunch[i], multi->wsa_event, 0);
-- 
2.7.4
@rmja
Copy link

rmja commented Jul 26, 2020

The build produces the same result :-/ - it still passes in around 55 seconds.

@mback2k
Copy link
Member

mback2k commented Jul 28, 2020

Putting this on hold for the next feature window, until #5633 has been successfully tracked down and solved.

@mback2k
Copy link
Member

mback2k commented Jul 28, 2020

@Togtja would you mind joining this PR by testing the builds above to see if they also affect the performance for you?

@Togtja
Copy link

Togtja commented Jul 29, 2020

@mback2k When building from this using CMake with vs build tools 2019, the performance seems to be exactly the same as 7.71.1 on downloads and uploads to the SFTP server. I am going to test with a few more variations of workload, but I suspect it's going to be the same.

While this PR and 7.71.1 is much faster to upload than 7.71.0, it is still a fair bit slower than Filezilla that I normally use. curl uses about twice the amount of time. For downloads, FileZilla is just a tad bit faster <10%

@mback2k
Copy link
Member

mback2k commented Jul 29, 2020

Please be more exact. Is 7.71.1 as fast as 7.70.0 for you and therefore this PR does also not affect that negatively? What about my patch above?

I don't think we want to compare with other tools like FilaZilla right now, we first want to make sure that curl is not impacted by this.

@Togtja
Copy link

Togtja commented Jul 29, 2020

Sorry, I did not compare my results to 7.70.0, However I have done so now, and this PR, your pre-compiled binaries (20200726 and 20200722), 7.70.0 and 7.71.1 all take about 30 seconds +/- 2s to upload 119Mb.

7.71.0 on the other hand does this in about 1,5 hours.

The testing I did was running
./curl.exe -k -T test_of_large_zip_upload.zip sftp://user:pass@hodt:port/Path/to/folder/test_of_large_zip_upload.zip
I ran this command 3 times for each build I had. Note that the PR, 7.70.0, 7.71.0 and 7.71.1 was built on my computer using CMake with libnssh2 and OpenSSL:
curl 7.7x.x (Windows) libcurl/7.7x.x OpenSSL/1.1.1g libssh2/1.9.0_DEV x depending if it was 7.70.0, 7.71.0 or 7.71.1

The comparison to FileZilla was just a side note.

@mback2k mback2k removed the on-hold label Jul 29, 2020
@mback2k
Copy link
Member

mback2k commented Jul 29, 2020

@Togtja thank you very much @rmja can you confirm this by doing the same tests, especially comparing against 7.70.0?

Copy link
Member

bagder left a comment

I cannot comment on the code as this uses lots of Windows magic that I don't know anything about.

I think the least positive thing with this code is the extreme amount new #ifdefs it adds, since it really creates a maze. Unfortunately I can't say I have any immediate ideas on how to remedy that.

If the code builds and doesn't cause regressions I'm fine with the change as I think it improves curl on Windows.

lib/multi.c Outdated
struct pollfd a_few_on_stack[NUM_POLLS_ON_STACK];
struct pollfd *ufds = &a_few_on_stack[0];
bool ufds_malloc = FALSE;
#else
int already_writable = 0;

This comment has been minimized.

@bagder

bagder Aug 3, 2020 Member

maybe just use a bool for this instead ?

This comment has been minimized.

@mback2k

mback2k Aug 3, 2020 Member

@rcombs would you mind applying my patch above, since it also addresses this?

if(select((int)sockbunch[i] + 1, NULL, &writefds, NULL,
&timeout) == 1)
already_writable++;
mask |= FD_WRITE;

This comment has been minimized.

@bagder

bagder Aug 3, 2020 Member

maybe this code could be made into a separate sub-function as the exact same logic is done again further down?

This comment has been minimized.

@mback2k

mback2k Aug 3, 2020 Member

@rcombs would you mind applying my patch above, since it also addresses this by using existing macros?

@mback2k
Copy link
Member

mback2k commented Aug 3, 2020

I cannot comment on the code as this uses lots of Windows magic that I don't know anything about.

I requested your review in order to get a general approval to do this change since it broke curl in the past. 😉

I think the least positive thing with this code is the extreme amount new #ifdefs it adds, since it really creates a maze. Unfortunately I can't say I have any immediate ideas on how to remedy that.

Yes, I also thought whether it would be better to just create a new Windows-specific wait function and use that instead. What do you think?

If the code builds and doesn't cause regressions I'm fine with the change as I think it improves curl on Windows.

We are not sure about this yet, as we are waiting on feedback from @rmja once he is available again (see #5633 (comment)).

@mback2k
Copy link
Member

mback2k commented Aug 9, 2020

Since @rmja's issue in #5633 seems unrelated to the previous or current version of this change, I think we can merge it during the next feature window. @rcombs what do you think about my patch above? I might just do a follow up PR once this PR has landed.

@rcombs
Copy link
Contributor Author

rcombs commented Aug 9, 2020

Patch overall looks good, but are the SOCKET_READABLE checks necessary? Seems like that'd add extraneous syscalls that shouldn't be needed, if I'm reading the Winsock docs correctly.

@mback2k
Copy link
Member

mback2k commented Aug 10, 2020

@rcombs I think they are due to the fact that FD_READ or FD_WRITE with FD_CLOSE is only signaled once for a socket and this way we can avoid waiting on an already closed socket by pre-checking it with select which transparently handles this for us (even on Windows).

This avoids using a pair of TCP ports to provide wakeup functionality
for every multi instance on Windows, where socketpair() is emulated
using a TCP socket on loopback which could in turn lead to socket
resource exhaustion.

A previous version of this patch failed to account for how in Winsock,
FD_WRITE is set only once when writing becomes possible and not again
until after a send has failed due to the buffer filling. This contrasts
to how FD_READ and FD_OOB continue to be set until the conditions they
refer to no longer apply. This meant that if a user wrote some data to
a socket, but not enough data to completely fill its send buffer, then
waited on that socket to become writable, we'd erroneously stall until
their configured timeout rather than returning immediately.

This version of the patch addresses that issue by checking each socket
we're waiting on to become writable with select() before the wait, and
zeroing the timeout if it's already writable.

Improved-By: Marc Hoersken <info@marc-hoersken.de>
@rcombs rcombs force-pushed the rcombs:winsock-redux branch from 8805a4b to a69d1a4 Aug 20, 2020
@rcombs
Copy link
Contributor Author

rcombs commented Aug 20, 2020

Alright, rebased and re-pushed with @mback2k's changes applied; think we should be good to go now.

@mback2k
Copy link
Member

mback2k commented Aug 20, 2020

@rcombs thanks, I will merge this once the release has cooled of a little and we can be sure there is no bugfix release needed.

@mback2k mback2k self-assigned this Aug 25, 2020
@mback2k mback2k closed this in d2a7d7c Aug 25, 2020
mback2k added a commit that referenced this pull request Aug 25, 2020
Check readiness of all sockets before waiting on them
to avoid locking in case the one-time event FD_WRITE
was already consumed by a previous wait operation.

More information about WinSock network events:
https://docs.microsoft.com/en-us/windows/win32/api/
   winsock2/nf-winsock2-wsaeventselect#return-value

Closes #5634
mback2k added a commit to mback2k/curl that referenced this pull request Aug 25, 2020
Update the ifdef-jungle to check for WinSock version 2.

Follow up to curl#5634
mback2k added a commit to mback2k/curl that referenced this pull request Aug 26, 2020
Update the ifdef-jungle to check for WinSock version 2.

Introduce version specific WinSock defines to ease checking.

Follow up to curl#5634
mback2k added a commit that referenced this pull request Aug 28, 2020
Learn from the way Cygwin handles and maps the WinSock events
to simulate correct and complete poll and select behaviour
according to Richard W. Stevens Network Programming book.

Reviewed-by: Jay Satiro
Reviewed-by: Marcel Raad

Follow up to #5634
Closes #5867
mback2k added a commit that referenced this pull request Sep 2, 2020
IPv6, telnet and now also the multi API require WinSock
version 2 which is available starting with Windows 95.

Therefore we think it is time to drop support for version 1.

Reviewed-by: Marcel Raad
Reviewed-by: Jay Satiro
Reviewed-by: Daniel Stenberg
Reviewed-by: Viktor Szakats

Follow up to #5634
Closes #5854
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.