[PATCH 0/1] MR2687: ntdll: Retry send on ECONNREFUSED in try_send().

[Paul Gofman (＠gofman)]

Fixes low FPS in Ghost Recon Breakpoint in dx11 mode.

[PATCH 1/1] ntdll: Retry send on ECONNREFUSED in try_send().

[Paul Gofman]

From: Paul Gofman

---
 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..fe45f5bac81 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(23456);
+
+    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)

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/ws2_32/tests/sock.c:
>      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(23456);

This is a bit risky since the port might be in use. Elsewhere we use the guaranteed-invalid port number 255.

[Zebediah Figura (＠zfigura)]

Sorry for the late review; I somehow managed to miss this patch.

The fact that we're trying to send twice for all socket types makes me a bit nervous, but it's probably okay. I guess there's not a particularly easy way to limit this to UDP.

[Paul Gofman (＠gofman)]

On Tue May  9 21:20:08 2023 +0000, Zebediah Figura wrote:
> Sorry for the late review; I somehow managed to miss this patch.
> The fact that we're trying to send twice for all socket types makes me a
> bit nervous, but it's probably okay. I guess there's not a particularly
> easy way to limit this to UDP.

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.

[Paul Gofman (＠gofman)]

On Tue May  9 19:40:49 2023 +0000, Zebediah Figura wrote:
> This is a bit risky since the port might be in use. Elsewhere we use the
> guaranteed-invalid port number 255.

Yeah, I worried that reserved port might behave differently WRT those ICMP replies and not reproduce the issue but it looks like it does.

[Paul Gofman (＠gofman)]

On Tue May  9 21:20:07 2023 +0000, Paul Gofman 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 also can imagine that in theory something between the lines of commit 4e6a5d62ad2bbccafa751812ca31eb9ffa25727e may happen (while I am not immediately sure how to trigger that), but in that case calling send() once again won't hurt too probably, even if to get a more sensible error code.