From 22678267cdb8fa519866c52dd91c20d82f54df30 Mon Sep 17 00:00:00 2001
From: Vishesh Yadav <vishesh_yadav@apple.com>
Date: Fri, 5 Jul 2019 16:27:17 -0700
Subject: [PATCH] fdbrpc: Don't drop idle connections from server

Instead try pinging the client and let that decide whether the client
is alive or not. Ideally, it should always be failed since a well
behaved client would have closed the connection.
---
 fdbrpc/FlowTransport.actor.cpp | 44 ++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/fdbrpc/FlowTransport.actor.cpp b/fdbrpc/FlowTransport.actor.cpp
index 5392cbede2..605edbbfc3 100644
--- a/fdbrpc/FlowTransport.actor.cpp
+++ b/fdbrpc/FlowTransport.actor.cpp
@@ -397,33 +397,39 @@ struct Peer : NonCopyable {
 	}
 
 	ACTOR static Future<Void> connectionMonitor( Peer *peer ) {
-		if (!peer->destination.isPublic()) {
-			// Don't send ping messages to clients. Instead monitor incoming client pings.
-			state double lastRefreshed = now();
-			state int64_t lastBytesReceived = peer->bytesReceived;
-			loop {
-				wait(delay(FLOW_KNOBS->CONNECTION_MONITOR_LOOP_TIME));
-				if (lastBytesReceived < peer->bytesReceived) {
-					lastRefreshed = now();
-					lastBytesReceived = peer->bytesReceived;
-				} else if (lastRefreshed < now() - FLOW_KNOBS->CONNECTION_MONITOR_IDLE_TIMEOUT *
-				                                       FLOW_KNOBS->CONNECTION_MONITOR_INCOMING_IDLE_MULTIPLIER) {
-					throw connection_idle();
-				}
-			}
-		}
-
 		state Endpoint remotePingEndpoint({ peer->destination }, WLTOKEN_PING_PACKET);
 		loop {
+			if (!FlowTransport::transport().isClient() && !peer->destination.isPublic()) {
+				// Don't send ping messages to clients unless necessary. Instead monitor incoming client pings.
+				state double lastRefreshed = now();
+				state int64_t lastBytesReceived = peer->bytesReceived;
+				loop {
+					wait(delay(FLOW_KNOBS->CONNECTION_MONITOR_LOOP_TIME));
+					if (lastBytesReceived < peer->bytesReceived) {
+						lastRefreshed = now();
+						lastBytesReceived = peer->bytesReceived;
+					} else if (lastRefreshed < now() - FLOW_KNOBS->CONNECTION_MONITOR_IDLE_TIMEOUT *
+							   FLOW_KNOBS->CONNECTION_MONITOR_INCOMING_IDLE_MULTIPLIER) {
+						// If we have not received anything in this period, client must have closed
+						// connection by now. Break loop to check if it is still alive by sending a ping.
+						break;
+					}
+				}
+			}
+
 			const bool pendingPacketsEmpty = peer->reliable.empty() && peer->unsent.empty();
 			if (pendingPacketsEmpty && (peer->lastConnectTime < now() - FLOW_KNOBS->CONNECTION_MONITOR_IDLE_TIMEOUT) &&
 			    (peer->lastDataPacketSentTime < now() - FLOW_KNOBS->CONNECTION_MONITOR_IDLE_TIMEOUT)) {
-				if (peer->peerReferences == 0)
+				if (peer->peerReferences == 0) {
 					throw connection_unreferenced();
-				else if (peer->destination.isPublic())
+				} else if (FlowTransport::transport().isClient() && peer->destination.isPublic()) {
+					// First condition is necessary because we may get here if we are server.
 					throw connection_idle();
+				}
 			}
 
+			wait (delayJittered(FLOW_KNOBS->CONNECTION_MONITOR_LOOP_TIME));
+
 			// TODO: Stop monitoring and close the connection with no onDisconnect requests outstanding
 			state ReplyPromise<Void> reply;
 			FlowTransport::transport().sendUnreliable( SerializeSource<ReplyPromise<Void>>(reply), remotePingEndpoint );
@@ -453,8 +459,6 @@ struct Peer : NonCopyable {
 					}
 				}
 			}
-
-			wait (delayJittered(FLOW_KNOBS->CONNECTION_MONITOR_LOOP_TIME));
 		}
 	}