# HG changeset patch
# User Patric Stout <truebrain@openttd.org>
# Date 2021-04-27 18:18:43
# Node ID 32bdda38037dadb1548dc50d93287c1f941dfa00
# Parent  53db62f40fa0ad672ba78d8ba12eeda6ca3fda80

Change: [Network] lower TCP connect() timeout to 3s (#9112)

Currently we use default OS timeout for TCP connections, which
is around 30s. 99% of the users will never notice this, but there
are a few cases where this is an issue:

- If you have a broken IPv6 connection, using Content Service is
  first tried over IPv6. Only after 30s it times out and tries
  IPv4. Nobody is waiting for that 30s.
- Upcoming STUN support has several methods of establishing a
  connection between client and server. This requires feedback
  from connect() to know if any method worked (they have to be
  tried one by one). With 30s, this would take a very long time.

What is good to mention, is that there is no good value here. Any
value will have edge-cases where the experience is suboptimal. But
with 3s we support most of the stable connections, and if it fails,
the user can just retry. On the other side of the spectrum, with 30s,
it means the user has no possibility to use the service. So worst case
we annoy a few users with them having the retry vs annoying a few
users which have no means of resolving the situation.

diff --git a/src/network/core/address.cpp b/src/network/core/address.cpp
--- a/src/network/core/address.cpp
+++ b/src/network/core/address.cpp
@@ -14,6 +14,8 @@
 
 #include "../../safeguards.h"
 
+static const int DEFAULT_CONNECT_TIMEOUT_SECONDS = 3; ///< Allow connect() three seconds to connect.
+
 /**
  * Get the hostname; in case it wasn't given the
  * IPv4 dotted representation is given.
@@ -322,23 +324,47 @@ static SOCKET ConnectLoopProc(addrinfo *
 
 	if (!SetNoDelay(sock)) DEBUG(net, 1, "[%s] setting TCP_NODELAY failed", type);
 
+	if (!SetNonBlocking(sock)) DEBUG(net, 0, "[%s] setting non-blocking mode failed", type);
+
 	int err = connect(sock, runp->ai_addr, (int)runp->ai_addrlen);
-#ifdef __EMSCRIPTEN__
-	/* Emscripten is asynchronous, and as such a connect() is still in
-	 * progress by the time the call returns. */
-	if (err != 0 && errno != EINPROGRESS)
-#else
-	if (err != 0)
-#endif
-	{
-		DEBUG(net, 1, "[%s] could not connect %s socket: %s", type, family, NetworkGetLastErrorString());
+	if (err != 0 && NetworkGetLastError() != EINPROGRESS) {
+		DEBUG(net, 1, "[%s] could not connect to %s over %s: %s", type, address, family, NetworkGetLastErrorString());
 		closesocket(sock);
 		return INVALID_SOCKET;
 	}
 
-	/* Connection succeeded */
-	if (!SetNonBlocking(sock)) DEBUG(net, 0, "[%s] setting non-blocking mode failed", type);
+	fd_set write_fd;
+	struct timeval tv;
+
+	FD_ZERO(&write_fd);
+	FD_SET(sock, &write_fd);
+
+	/* Wait for connect() to either connect, timeout or fail. */
+	tv.tv_usec = 0;
+	tv.tv_sec = DEFAULT_CONNECT_TIMEOUT_SECONDS;
+	int n = select(FD_SETSIZE, NULL, &write_fd, NULL, &tv);
+	if (n < 0) {
+		DEBUG(net, 1, "[%s] could not connect to %s: %s", type, address, NetworkGetLastErrorString());
+		closesocket(sock);
+		return INVALID_SOCKET;
+	}
 
+	/* If no fd is selected, the timeout has been reached. */
+	if (n == 0) {
+		DEBUG(net, 1, "[%s] timed out while connecting to %s", type, address);
+		closesocket(sock);
+		return INVALID_SOCKET;
+	}
+
+	/* Retrieve last error, if any, on the socket. */
+	err = GetSocketError(sock);
+	if (err != 0) {
+		DEBUG(net, 1, "[%s] could not connect to %s: %s", type, address, NetworkGetErrorString(err));
+		closesocket(sock);
+		return INVALID_SOCKET;
+	}
+
+	/* Connection succeeded. */
 	DEBUG(net, 1, "[%s] connected to %s", type, address);
 
 	return sock;
diff --git a/src/network/core/os_abstraction.h b/src/network/core/os_abstraction.h
--- a/src/network/core/os_abstraction.h
+++ b/src/network/core/os_abstraction.h
@@ -33,6 +33,8 @@
 #define EWOULDBLOCK WSAEWOULDBLOCK
 #undef ECONNRESET
 #define ECONNRESET WSAECONNRESET
+#undef EINPROGRESS
+#define EINPROGRESS WSAEWOULDBLOCK
 
 const char *NetworkGetErrorString(int error);
 
@@ -230,6 +232,20 @@ static inline bool SetNoDelay(SOCKET d)
 #endif
 }
 
+/**
+ * Get the error from a socket, if any.
+ * @param d The socket to get the error from.
+ * @return The errno on the socket.
+ */
+static inline int GetSocketError(SOCKET d)
+{
+	int err;
+	socklen_t len = sizeof(err);
+	getsockopt(d, SOL_SOCKET, SO_ERROR, (char *)&err, &len);
+
+	return err;
+}
+
 /* Make sure these structures have the size we expect them to be */
 static_assert(sizeof(in_addr)  ==  4); ///< IPv4 addresses should be 4 bytes.
 static_assert(sizeof(in6_addr) == 16); ///< IPv6 addresses should be 16 bytes.