[PATCH v4 0/2] MR2215: winsock: Fix two connect() issues.
I was mainly after the issue fixed by the second patch but stomped on another one while testing that and that one looks even more unfortunate to me. 1. When connect() is used on blocking socket and fails for whatever reason, next connect() will make a new independent connection attempt (both on Windows and Unix). In non-blocking mode however (and Unix sockets are always in non-blocking mode in Wine) on Linux the next connect() will always return an error (unlike Windows which will make a new connect attempt if the previous one completed and failed). So in Wine now even for blocking socket connect() will fail if previous attempt failed and return WSAECONNABORTED. The error returned by Linux connect() will be ECONNABORTED if getsockopt(SO_ERROR) was called on the socket after connect completed with failure, and true connect() fail error if getsockopt wasn't called. 2. connect() on blocking socket times out after ~21sec on Windows and after ~130 sec on Linux. That is the case when peer doesn't actively reject connection but just never replies anything. That is, client sent SYN and never heard back. So much longer timeout leads to what is seen as hang on exit in one game, but probably more important that it affect much more in various apps provoking timeouts or huge delays not just on exit. I did not find a way to set connect() timeout for socket on Windows. That difference mostly corresponds to number of SYN send retries. That is 7 on Linux by default and 4 on Windows (the delay between attempts starts from 1s and is doubled on each next attempt). There is TCP_SYNCNT socket option (Linux specific now, unfortunately) which allows to set the number of retries. Setting that to 4 yields 32 sec timeout (vs 21 sec on Windows). The remaining difference is probably due to different amount of time waited after last SYN is sent. There is also TCP_USER_TIMEOUT parameter which allows to effectively set the delay exactly, but it might interfere with SO_KEEPALIVE somehow and I hope that 32 vs 21 is not that critical already as 130 vs 21 and probably we can avoid complicating the thing with it. -- v4: server: Set TCP SYN count on sockets. server: Retry socket connection on ECONNABORTED error. https://gitlab.winehq.org/wine/wine/-/merge_requests/2215
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/tests/sock.c | 41 +++++++++++++++++++++++++--------------- server/sock.c | 11 +++++++++++ 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 58dcf9e7f4e..17298ca4136 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -4352,13 +4352,7 @@ static void test_select(void) ret = connect(fdWrite, (const struct sockaddr *)&invalid_addr, sizeof(invalid_addr)); ok(ret == -1, "got %d\n", ret); - todo_wine ok(WSAGetLastError() == WSAEWOULDBLOCK, "got error %u\n", WSAGetLastError()); - if (WSAGetLastError() == WSAECONNABORTED) - { - ret = connect(fdWrite, (const struct sockaddr *)&invalid_addr, sizeof(invalid_addr)); - ok(ret == -1, "got %d\n", ret); - ok(WSAGetLastError() == WSAEWOULDBLOCK, "got error %u\n", WSAGetLastError()); - } + ok(WSAGetLastError() == WSAEWOULDBLOCK, "got error %u\n", WSAGetLastError()); len = sizeof(id); id = 0xdeadbeef; @@ -4380,13 +4374,7 @@ static void test_select(void) ok(!ret, "got error %u\n", WSAGetLastError()); ret = connect(fdWrite, (const struct sockaddr *)&address, sizeof(address)); ok(ret == -1, "got %d\n", ret); - todo_wine ok(WSAGetLastError() == WSAEWOULDBLOCK, "got error %u\n", WSAGetLastError()); - if (WSAGetLastError() == WSAECONNABORTED) - { - ret = connect(fdWrite, (const struct sockaddr *)&address, sizeof(address)); - ok(ret == -1, "got %d\n", ret); - ok(WSAGetLastError() == WSAEWOULDBLOCK, "got error %u\n", WSAGetLastError()); - } + ok(WSAGetLastError() == WSAEWOULDBLOCK, "got error %u\n", WSAGetLastError()); FD_ZERO_ALL(); FD_SET(fdWrite, &readfds); @@ -8320,7 +8308,6 @@ static void test_connect(void) closesocket(connector); closesocket(acceptor); - closesocket(listener); tcp_socketpair(&connector, &acceptor); @@ -8380,6 +8367,30 @@ static void test_connect(void) WSACloseEvent(overlapped.hEvent); closesocket(connector); + + /* Test connect after previous connect attempt failure. */ + connector = socket(AF_INET, SOCK_STREAM, 0); + ok(connector != INVALID_SOCKET, "failed to create socket, error %u\n", WSAGetLastError()); + + conaddress.sin_addr.s_addr = htonl(INADDR_LOOPBACK); + conaddress.sin_port = htons(255); + iret = connect(connector, (struct sockaddr *)&conaddress, sizeof(conaddress)); + ok(iret == -1, "connection succeeded.\n"); + + ok(WSAGetLastError() == WSAECONNREFUSED, "got error %u\n", WSAGetLastError()); + set_blocking( connector, FALSE ); + iret = getsockname(listener, (struct sockaddr*)&address, &addrlen); + ok(!iret, "failed to get address, error %u\n", WSAGetLastError()); + + iret = connect(connector, (struct sockaddr *)&address, sizeof(address)); + ok(iret == -1 && WSAGetLastError() == WSAEWOULDBLOCK, "unexpected iret %d, error %d.\n", + iret, WSAGetLastError()); + acceptor = accept(listener, NULL, NULL); + ok(acceptor != INVALID_SOCKET, "could not accept socket error %d\n", WSAGetLastError()); + + closesocket(acceptor); + closesocket(connector); + closesocket(listener); } static void test_AcceptEx(void) diff --git a/server/sock.c b/server/sock.c index a64cb22404e..610e0a94cc0 100644 --- a/server/sock.c +++ b/server/sock.c @@ -2602,6 +2602,17 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) unix_addr.in.sin_addr.s_addr = htonl( INADDR_LOOPBACK ); ret = connect( unix_fd, &unix_addr.addr, unix_len ); + if (ret < 0 && errno == ECONNABORTED) + { + /* On Linux with nonblocking socket if the previous connect() failed for any reason (including + * timeout), next connect will fail. If the error code was queried by getsockopt( SO_ERROR ) + * the error code returned now is ECONNABORTED (otherwise that is the actual connect() failure + * error code). If we got here after previous connect attempt on the socket that means + * we already queried SO_ERROR in sock_error(), so retrying on ECONNABORTED only is + * sufficient. */ + ret = connect( unix_fd, &unix_addr.addr, unix_len ); + } + if (ret < 0 && errno != EINPROGRESS) { set_error( sock_get_ntstatus( errno ) ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2215
From: Paul Gofman <pgofman(a)codeweavers.com> --- server/sock.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/server/sock.c b/server/sock.c index 610e0a94cc0..088e6d63079 100644 --- a/server/sock.c +++ b/server/sock.c @@ -39,6 +39,9 @@ #ifdef HAVE_NETINET_IN_H # include <netinet/in.h> #endif +#ifdef HAVE_NETINET_TCP_H +# include <netinet/tcp.h> +#endif #include <poll.h> #include <sys/time.h> #include <sys/types.h> @@ -1921,9 +1924,12 @@ static int init_socket( struct sock *sock, int family, int type, int protocol ) if (is_tcp_socket( sock )) { - int reuse = 1; - - setsockopt( sockfd, SOL_SOCKET, SO_REUSEADDR, &reuse, sizeof(reuse) ); + value = 1; + setsockopt( sockfd, SOL_SOCKET, SO_REUSEADDR, &value, sizeof(value) ); +#ifdef TCP_SYNCNT + value = 4; + setsockopt( sockfd, IPPROTO_TCP, TCP_SYNCNT, &value, sizeof(value) ); +#endif } if (sock->fd) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2215
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=129636 Your paranoid android. === debian11 (32 bit report) === dinput: device8.c:2030: Test failed: 0x800: WaitForSingleObject returned 0x102 device8.c:2036: Test failed: 0x800: WaitForSingleObject returned 0x102 device8.c:2036: Test failed: 0x800: WaitForSingleObject returned 0x102 device8.c:2041: Test failed: 0x800: GetDeviceData returned 0
v4: - also remove now unneeded fallback path in test. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2215#note_24724
This is still missing interactive tests in afd.ok I'm afraid. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2215#note_24916
participants (4)
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)
-
Zebediah Figura (＠zfigura)