Refactoring: unlinkClient() added to lower freeClient() complexity.

3 files changed, 48 insertions, 51 deletions

diff --git a/src/networking.c b/src/networking.c
index d02d02b58..6dc4864bf 100644
--- a/src/networking.c
+++ b/src/networking.c
@@ -685,12 +685,50 @@ void disconnectSlaves(void) {
     }
 }
 
-void freeClient(client *c) {
+/* Remove the specified client from global lists where the client could
+ * be referenced, not including the Pub/Sub channels.
+ * This is used by freeClient() and replicationCacheMaster(). */
+void unlinkClient(client *c) {
     listNode *ln;
 
-    /* If this is marked as current client unset it */
+    /* If this is marked as current client unset it. */
     if (server.current_client == c) server.current_client = NULL;
 
+    /* Certain operations must be done only if the client has an active socket.
+     * If the client was already unlinked or if it's a "fake client" the
+     * fd is already set to -1. */
+    if (c->fd != -1) {
+        /* Remove from the list of active clients. */
+        ln = listSearchKey(server.clients,c);
+        serverAssert(ln != NULL);
+        listDelNode(server.clients,ln);
+
+        /* Unregister async I/O handlers and close the socket. */
+        aeDeleteFileEvent(server.el,c->fd,AE_READABLE);
+        aeDeleteFileEvent(server.el,c->fd,AE_WRITABLE);
+        close(c->fd);
+        c->fd = -1;
+    }
+
+    /* Remove from the list of pending writes if needed. */
+    if (c->flags & CLIENT_PENDING_WRITE) {
+        ln = listSearchKey(server.clients_pending_write,c);
+        serverAssert(ln != NULL);
+        listDelNode(server.clients_pending_write,ln);
+    }
+
+    /* When client was just unblocked because of a blocking operation,
+     * remove it from the list of unblocked clients. */
+    if (c->flags & CLIENT_UNBLOCKED) {
+        ln = listSearchKey(server.unblocked_clients,c);
+        serverAssert(ln != NULL);
+        listDelNode(server.unblocked_clients,ln);
+    }
+}
+
+void freeClient(client *c) {
+    listNode *ln;
+
     /* If it is our master that's beging disconnected we should make sure
      * to cache the state to try a partial resynchronization later.
      *
@@ -732,36 +770,14 @@ void freeClient(client *c) {
     dictRelease(c->pubsub_channels);
     listRelease(c->pubsub_patterns);
 
-    /* Close socket, unregister events, and remove list of replies and
-     * accumulated arguments. */
-    if (c->fd != -1) {
-        aeDeleteFileEvent(server.el,c->fd,AE_READABLE);
-        aeDeleteFileEvent(server.el,c->fd,AE_WRITABLE);
-        close(c->fd);
-    }
+    /* Free data structures. */
     listRelease(c->reply);
     freeClientArgv(c);
 
-    /* Remove from the list of clients */
-    if (c->fd != -1) {
-        ln = listSearchKey(server.clients,c);
-        serverAssert(ln != NULL);
-        listDelNode(server.clients,ln);
-    }
-
-    /* Remove from the list of pending writes if needed. */
-    if (c->flags & CLIENT_PENDING_WRITE) {
-        ln = listSearchKey(server.clients_pending_write,c);
-        listDelNode(server.clients_pending_write,ln);
-    }
-
-    /* When client was just unblocked because of a blocking operation,
-     * remove it from the list of unblocked clients. */
-    if (c->flags & CLIENT_UNBLOCKED) {
-        ln = listSearchKey(server.unblocked_clients,c);
-        serverAssert(ln != NULL);
-        listDelNode(server.unblocked_clients,ln);
-    }
+    /* Unlink the client: this will close the socket, remove the I/O
+     * handlers, and remove references of the client from different
+     * places where active clients may be referenced. */
+    unlinkClient(c);
 
     /* Master/slave cleanup Case 1:
      * we lost the connection with a slave. */
diff --git a/src/replication.c b/src/replication.c
index 6b5f44dd8..7ab646362 100644
--- a/src/replication.c
+++ b/src/replication.c
@@ -1846,36 +1846,16 @@ void replicationSendAck(void) {
  * handshake in order to reactivate the cached master.
  */
 void replicationCacheMaster(client *c) {
-    listNode *ln;
-
     serverAssert(server.master != NULL && server.cached_master == NULL);
     serverLog(LL_NOTICE,"Caching the disconnected master state.");
 
-    /* Remove from the list of clients, we don't want this client to be
-     * listed by CLIENT LIST or processed in any way by batch operations. */
-    ln = listSearchKey(server.clients,c);
-    serverAssert(ln != NULL);
-    listDelNode(server.clients,ln);
-
-    /* Remove from the list of clients with pending writes as well. */
-    if (c->flags & CLIENT_PENDING_WRITE) {
-        ln = listSearchKey(server.clients_pending_write,c);
-        if (ln) listDelNode(server.clients_pending_write,ln);
-    }
+    /* Unlink the client from the server structures. */
+    unlinkClient(c);
 
     /* Save the master. Server.master will be set to null later by
      * replicationHandleMasterDisconnection(). */
     server.cached_master = server.master;
 
-    /* Remove the event handlers and close the socket. We'll later reuse
-     * the socket of the new connection with the master during PSYNC. */
-    aeDeleteFileEvent(server.el,c->fd,AE_READABLE);
-    aeDeleteFileEvent(server.el,c->fd,AE_WRITABLE);
-    close(c->fd);
-
-    /* Set fd to -1 so that we can safely call freeClient(c) later. */
-    c->fd = -1;
-
     /* Invalidate the Peer ID cache. */
     if (c->peerid) {
         sdsfree(c->peerid);
diff --git a/src/server.h b/src/server.h
index 400f1ff80..b5a248292 100644
--- a/src/server.h
+++ b/src/server.h
@@ -1112,6 +1112,7 @@ int clientsArePaused(void);
 int processEventsWhileBlocked(void);
 void handleClientsWithPendingWrites(void);
 int clientHasPendingReplies(client *c);
+void unlinkClient(client *c);
 
 #ifdef __GNUC__
 void addReplyErrorFormat(client *c, const char *fmt, ...)