multi: implement wait using winsock events #5634

rcombs · 2020-06-30T20:34:56Z

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).

mback2k · 2020-06-30T21:13:58Z

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

mback2k

LGTM

mback2k · 2020-07-05T19:46:08Z

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.

MarcelRaad

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 · 2020-07-06T05:48:54Z

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; 
        
             }

ngg · 2020-07-10T18:09:28Z

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 · 2020-07-14T18:01:24Z

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

jay · 2020-07-15T06:59:56Z

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

bagder · 2020-07-15T12:09:14Z

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 · 2020-07-15T16:09:50Z

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 · 2020-07-15T20:04:38Z

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

mback2k · 2020-07-21T18:48:44Z

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 · 2020-07-21T19:30:10Z

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

mback2k · 2020-07-21T20:15:49Z

Okay, I can probably provide a debug build tomorrow.

mback2k · 2020-07-22T18:20:27Z

@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 · 2020-07-23T07:15:19Z

@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 · 2020-07-23T07:18:11Z

@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 · 2020-07-23T07:35:53Z

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

Togtja · 2020-07-29T08:25:13Z

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 · 2020-07-29T08:42:45Z

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

bagder

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.

bagder · 2020-08-03T09:08:43Z

lib/multi.c

  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;


maybe just use a bool for this instead ?

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

bagder · 2020-08-03T09:11:36Z

lib/multi.c

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


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

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

mback2k · 2020-08-03T13:59:50Z

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 · 2020-08-09T13:19:45Z

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 · 2020-08-09T15:25:05Z

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 · 2020-08-10T05:39:29Z

@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 · 2020-08-20T04:54:58Z

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

mback2k · 2020-08-20T07:19:55Z

@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.

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

Update the ifdef-jungle to check for WinSock version 2. Follow up to curl#5634

Update the ifdef-jungle to check for WinSock version 2. Introduce version specific WinSock defines to ease checking. Follow up to curl#5634

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

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

This reverts commit 2260e0e, also restoring previous follow up changes which were reverted. Authored-by: rcombs on github Authored-by: Marc Hörsken Restores curl#5634 Reverts curl#6281 Part of curl#6245

This reverts commit 2260e0e, also restoring previous follow up changes which were reverted. Authored-by: rcombs on github Authored-by: Marc Hörsken Reviewed-by: Jay Satiro Reviewed-by: Marcel Raad Restores #5634 Reverts #6281 Part of #6245

rcombs force-pushed the winsock-redux branch from 9981a3f to 756c553 Compare June 30, 2020 20:51

mback2k reviewed Jul 1, 2020

View reviewed changes

lib/multi.c Outdated Show resolved Hide resolved

mback2k reviewed Jul 1, 2020

View reviewed changes

lib/multi.c Outdated Show resolved Hide resolved

rcombs force-pushed the winsock-redux branch 3 times, most recently from f62a6dc to 8805a4b Compare July 1, 2020 08:36

mback2k approved these changes Jul 5, 2020

View reviewed changes

mback2k requested review from MarcelRaad and jay July 5, 2020 19:39

MarcelRaad approved these changes Jul 5, 2020

View reviewed changes

jay added the Windows Windows-specific label Jul 6, 2020

mback2k mentioned this pull request Jul 6, 2020

cURL 7.71 sftp upload slow #5633

Closed

mback2k requested a review from bagder July 14, 2020 18:01

mback2k mentioned this pull request Jul 14, 2020

select: fix poll-based check not detecting connect failure #5509

Closed

mback2k removed the on-hold label Jul 29, 2020

bagder mentioned this pull request Jul 30, 2020

CURLMOPT_SOCKETFUNCTION called before CURLOPT_OPENSOCKETFUNCTION #5747

Closed

bagder reviewed Aug 3, 2020

View reviewed changes

rcombs force-pushed the winsock-redux branch from 8805a4b to a69d1a4 Compare August 20, 2020 04:54

mback2k self-assigned this Aug 25, 2020

mback2k closed this in d2a7d7c Aug 25, 2020

mback2k added a commit to mback2k/curl that referenced this pull request Aug 25, 2020

multi: socket readiness pre-check requires WinSock version 2

aa13ca8

Update the ifdef-jungle to check for WinSock version 2. Follow up to curl#5634

mback2k mentioned this pull request Aug 25, 2020

win32: drop support for WinSock version 1, require version 2 #5854

Closed

mback2k mentioned this pull request Aug 26, 2020

multi: handle connection state winsock events #5867

Closed

jay mentioned this pull request Oct 30, 2020

Poor upload performance in Windows #6146

Closed

mback2k mentioned this pull request Dec 16, 2020

multi: implement waiting and wakeup using winsock events #6245

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: implement wait using winsock events #5634

multi: implement wait using winsock events #5634

rcombs commented Jun 30, 2020

mback2k commented Jun 30, 2020

mback2k left a comment

mback2k commented Jul 5, 2020

MarcelRaad left a comment

mback2k commented Jul 6, 2020

ngg commented Jul 10, 2020

mback2k commented Jul 14, 2020

jay commented Jul 15, 2020

bagder commented Jul 15, 2020

mback2k commented Jul 15, 2020

rmja commented Jul 15, 2020

mback2k commented Jul 21, 2020

rmja commented Jul 21, 2020

mback2k commented Jul 21, 2020

mback2k commented Jul 22, 2020 •

edited

rmja commented Jul 23, 2020

mback2k commented Jul 23, 2020 •

edited

rmja commented Jul 23, 2020

Togtja commented Jul 29, 2020

mback2k commented Jul 29, 2020

bagder left a comment

bagder Aug 3, 2020

mback2k Aug 3, 2020

bagder Aug 3, 2020 •

edited

mback2k Aug 3, 2020

mback2k commented Aug 3, 2020 •

edited

mback2k commented Aug 9, 2020

rcombs commented Aug 9, 2020

mback2k commented Aug 10, 2020 •

edited

rcombs commented Aug 20, 2020

mback2k commented Aug 20, 2020

multi: implement wait using winsock events #5634

multi: implement wait using winsock events #5634

Conversation

rcombs commented Jun 30, 2020

mback2k commented Jun 30, 2020

mback2k left a comment

Choose a reason for hiding this comment

mback2k commented Jul 5, 2020

MarcelRaad left a comment

Choose a reason for hiding this comment

mback2k commented Jul 6, 2020

ngg commented Jul 10, 2020

mback2k commented Jul 14, 2020

jay commented Jul 15, 2020

bagder commented Jul 15, 2020

mback2k commented Jul 15, 2020

rmja commented Jul 15, 2020

mback2k commented Jul 21, 2020

rmja commented Jul 21, 2020

mback2k commented Jul 21, 2020

mback2k commented Jul 22, 2020 • edited

rmja commented Jul 23, 2020

mback2k commented Jul 23, 2020 • edited

rmja commented Jul 23, 2020

Togtja commented Jul 29, 2020

mback2k commented Jul 29, 2020

bagder left a comment

Choose a reason for hiding this comment

bagder Aug 3, 2020

Choose a reason for hiding this comment

mback2k Aug 3, 2020

Choose a reason for hiding this comment

bagder Aug 3, 2020 • edited

Choose a reason for hiding this comment

mback2k Aug 3, 2020

Choose a reason for hiding this comment

mback2k commented Aug 3, 2020 • edited

mback2k commented Aug 9, 2020

rcombs commented Aug 9, 2020

mback2k commented Aug 10, 2020 • edited

rcombs commented Aug 20, 2020

mback2k commented Aug 20, 2020

mback2k commented Jul 22, 2020 •

edited

mback2k commented Jul 23, 2020 •

edited

bagder Aug 3, 2020 •

edited

mback2k commented Aug 3, 2020 •

edited

mback2k commented Aug 10, 2020 •

edited