[PATCH v2 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. -- v2: 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 | 25 ++++++++++++++++++++++++- server/sock.c | 11 +++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 58dcf9e7f4e..0c24bbd3bbf 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -8320,7 +8320,6 @@ static void test_connect(void) closesocket(connector); closesocket(acceptor); - closesocket(listener); tcp_socketpair(&connector, &acceptor); @@ -8380,6 +8379,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
v2: - Use port 255 in test at which connect() should always fail. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2215#note_24596
This fixes a couple of interactive-only tests, could you please remove the todo's from those? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2215#note_24720
participants (3)
-
Paul Gofman -
Paul Gofman (＠gofman)
-
Zebediah Figura (＠zfigura)