[PATCH v2 0/1] MR2687: ntdll: Retry send on ECONNREFUSED in try_send().
Fixes low FPS in Ghost Recon Breakpoint in dx11 mode. -- v2: ntdll: Retry send on ECONNREFUSED in try_send(). https://gitlab.winehq.org/wine/wine/-/merge_requests/2687
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/unix/socket.c | 12 ++++++++++++ dlls/ws2_32/tests/sock.c | 25 ++++++++++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index ac351a17a70..4e706323a0a 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -984,6 +984,7 @@ static NTSTATUS try_send( int fd, struct async_send_ioctl *async ) { union unix_sockaddr unix_addr; struct msghdr hdr; + int attempt = 0; ssize_t ret; memset( &hdr, 0, sizeof(hdr) ); @@ -1028,6 +1029,17 @@ static NTSTATUS try_send( int fd, struct async_send_ioctl *async ) else if (errno != EINTR) { if (errno != EWOULDBLOCK) WARN( "sendmsg: %s\n", strerror( errno ) ); + + /* ECONNREFUSED may be returned if this is connected datagram socket and the system received + * ICMP "destination port unreachable" message from the peer. That is ignored + * on Windows. The first sendmsg() will clear the error in this case and the next + * call should succeed. */ + if (!attempt && errno == ECONNREFUSED) + { + ++attempt; + continue; + } + return sock_errno_to_status( errno ); } } diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 1125d4519d3..cddc4c125ff 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -2962,7 +2962,9 @@ static void test_UDP(void) /* peer 0 receives data from all other peers */ struct sock_info peer[NUM_UDP_PEERS]; char buf[16]; - int ss, i, n_recv, n_sent; + int ss, i, n_recv, n_sent, ret; + struct sockaddr_in addr; + int sock; memset (buf,0,sizeof(buf)); for ( i = NUM_UDP_PEERS - 1; i >= 0; i-- ) { @@ -3000,6 +3002,27 @@ static void test_UDP(void) ok ( n_recv == sizeof(buf), "UDP: recvfrom() received wrong amount of data or socket error: %d\n", n_recv ); ok ( memcmp ( &peer[0].peer.sin_port, buf, sizeof(peer[0].addr.sin_port) ) == 0, "UDP: port numbers do not match\n" ); } + + sock = socket( AF_INET, SOCK_DGRAM, IPPROTO_UDP); + ok( sock != INVALID_SOCKET, "got error %u.\n", WSAGetLastError() ); + + memset( &addr, 0, sizeof(addr) ); + addr.sin_family = AF_INET; + addr.sin_addr.s_addr = inet_addr("127.0.0.1"); + addr.sin_port = htons(255); + + ret = connect( sock, (struct sockaddr *)&addr, sizeof(addr) ); + ok( !ret, "got error %u.\n", WSAGetLastError() ); + + /* Send to UDP socket succeeds even if the packets are not received and the network is replying with + * "destination port unreachable" ICMP messages. */ + for (i = 0; i < 10; ++i) + { + ret = send( sock, buf, sizeof(buf), 0 ); + ok( ret == sizeof(buf), "got %d, error %u.\n", ret, WSAGetLastError() ); + } + + closesocket(sock); } static void test_WSASocket(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2687
v2: - Use port 255 in test. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2687#note_32383
We'd also need to limit it to connected UDP then which both adds complications, if not an extra server call. But I hope we might rely on the sockets not giving ECONNREFUSED for send() in any other cases, error codes are documented and breaking those in Unix kernel will probably break a lot of things anyway.
I've never managed to find more than vague documentation for any error handling when it comes to sockets; do you have a better source? I'll admit "breaking those will probably break a lot of things" is not a particularly comforting thought. At this point I'm pretty well convinced that nobody really knows how to handle network error conditions, and that's why network software is universally so bad... I'm not too scared to sign off on this patch, but it's something that we'll have to watch out for. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2687#note_32447
This merge request was approved by Zebediah Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2687
On Wed May 10 16:11:02 2023 +0000, Zebediah Figura wrote:
We'd also need to limit it to connected UDP then which both adds complications, if not an extra server call. But I hope we might rely on the sockets not giving ECONNREFUSED for send() in any other cases, error codes are documented and breaking those in Unix kernel will probably break a lot of things anyway. I've never managed to find more than vague documentation for any error handling when it comes to sockets; do you have a better source? I'll admit "breaking those will probably break a lot of things" is not a particularly comforting thought. At this point I'm pretty well convinced that nobody really knows how to handle network error conditions, and that's why network software is universally so bad... I'm not too scared to sign off on this patch, but it's something that we'll have to watch out for. No, I also don't have a particularly great source for socket error handling. I first found the explanation of ECONNREFUSED w/sendmsg UDP here: https://docs.oracle.com/cd/E36784_01/html/E36875/sendmsg-3socket.html. As a more pristine source, this can probably be concluded from https://www.rfc-editor.org/rfc/rfc1122#page-79 (p. 4.1.3.3 suggesting that UDP should relay ICMP errors to user). And the exact code is probably a mapping of ICMP error code. And we know from the tests that ICMP is ignored for UDP on Windows.
I only wanted to say that we already rely on specific error codes in many places and that is hardly avoidable, even if lack of clear and understandable specification is concerning. My source of optimism is only that as it looks to me historically Unix socket implementations were taking de-facto behaviour compatibility seriously (and that includes error codes). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2687#note_32454
participants (3)
-
Paul Gofman -
Paul Gofman (＠gofman)
-
Zebediah Figura (＠zfigura)