From 4a4fbcef8ac37505a571b2b3a6c17d39ced5a801 Mon Sep 17 00:00:00 2001
From: Amos Jeffries <squid3@treenet.co.nz>
Date: Mon, 16 Nov 2015 19:26:01 -0800
Subject: [PATCH] Use connIsFinished() when a transaction is completed
 successfully

initiateClose() may sound okay, but it actually is the error handling logic.
It will terminate the ConnStateData with an erro rmessage, leaving the completed
request in the pipeline which in turn will result in *_ABORTED being logged for
all requests with Connection:close headers even if they are cleanly finished.

connIsFinished() is (now) the clean way to finish ClientSocketContext objects
lifetime regardless of whether keep-alive is needed. The ConnStateData::kick()
will now handle that so we do not even need to call keepaliveNextRequest().

Remove the now unused ClientSocketContext::keepaliveNextRequest().
---
 src/client_side.cc            | 27 +++++++++++----------------
 src/client_side.h             |  1 -
 src/servers/FtpServer.cc      |  2 +-
 src/tests/stub_client_side.cc |  1 -
 4 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/src/client_side.cc b/src/client_side.cc
index 6bb345db10..14087421da 100644
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -52,7 +52,7 @@
  * data, or sending it.
  *
  \par
- * ClientKeepAliveNextRequest will then detect the presence of data in
+ * ConnStateData::kick() will then detect the presence of data in
  * the next ClientHttpRequest, and will send it, restablishing the
  * data flow.
  */
@@ -1427,19 +1427,14 @@ ClientSocketContextPushDeferredIfNeeded(ClientSocketContext::Pointer deferredReq
      */
 }
 
-/// called when we have successfully finished writing the response
-void
-ClientSocketContext::keepaliveNextRequest()
-{
-    debugs(33, 3, "ConnnStateData(" << http->getConn()->clientConnection << "), Context(" << clientConnection << ")");
-
-    // mark ourselves as completed
-    connIsFinished();
-}
-
 void
 ConnStateData::kick()
 {
+    if (!Comm::IsConnOpen(clientConnection)) {
+        debugs(33, 2, clientConnection << " Connection was closed");
+        return;
+    }
+
     if (pinning.pinned && !Comm::IsConnOpen(pinning.serverConnection)) {
         debugs(33, 2, clientConnection << " Connection was pinned but server side gone. Terminating client connection");
         clientConnection->close();
@@ -1704,6 +1699,7 @@ ClientSocketContext::doClose()
 void
 ClientSocketContext::initiateClose(const char *reason)
 {
+    debugs(33, 4, clientConnection << " because " << reason);
     http->getConn()->stopSending(reason); // closes ASAP
 }
 
@@ -1760,10 +1756,9 @@ ClientSocketContext::writeComplete(const Comm::ConnectionPointer &conn, char *,
 
     case STREAM_COMPLETE:
         debugs(33, 5, conn << " Stream complete, keepalive is " << http->request->flags.proxyKeepalive);
-        if (http->request->flags.proxyKeepalive)
-            keepaliveNextRequest();
-        else
-            initiateClose("STREAM_COMPLETE NOKEEPALIVE");
+        if (!http->request->flags.proxyKeepalive)
+            clientConnection->close();
+        connIsFinished();
         return;
 
     case STREAM_UNPLANNED_COMPLETE:
@@ -4717,7 +4712,7 @@ ConnStateData::clientPinnedConnectionRead(const CommIoCbParams &io)
     pinning.serverConnection->close();
 
     // If we are still sending data to the client, do not close now. When we are done sending,
-    // ClientSocketContext::keepaliveNextRequest() checks pinning.serverConnection and will close.
+    // ConnStateData::kick() checks pinning.serverConnection and will close.
     // However, if we are idle, then we must close to inform the idle client and minimize races.
     if (clientIsIdle && clientConnection != NULL)
         clientConnection->close();
diff --git a/src/client_side.h b/src/client_side.h
index f315616612..2e9c047d46 100644
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -76,7 +76,6 @@ public:
     ~ClientSocketContext();
     bool startOfOutput() const;
     void writeComplete(const Comm::ConnectionPointer &conn, char *bufnotused, size_t size, Comm::Flag errflag);
-    void keepaliveNextRequest();
 
     Comm::ConnectionPointer clientConnection; /// details about the client connection socket.
     ClientHttpRequest *http;    /* we pretend to own that job */
diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc
index 5c740a1cd0..bd1be2db2c 100644
--- a/src/servers/FtpServer.cc
+++ b/src/servers/FtpServer.cc
@@ -1274,7 +1274,7 @@ Ftp::Server::wroteReply(const CommIoCbParams &io)
         changeState(fssConnected, "Ftp::Server::wroteReply");
         if (bodyParser)
             finishDechunkingRequest(false);
-        context->keepaliveNextRequest();
+        context->connIsFinished();
         return;
     }
 }
diff --git a/src/tests/stub_client_side.cc b/src/tests/stub_client_side.cc
index e4f841aed7..bf3a393694 100644
--- a/src/tests/stub_client_side.cc
+++ b/src/tests/stub_client_side.cc
@@ -16,7 +16,6 @@
 //ClientSocketContext::~ClientSocketContext() STUB
 bool ClientSocketContext::startOfOutput() const STUB_RETVAL(false)
 void ClientSocketContext::writeComplete(const Comm::ConnectionPointer &conn, char *bufnotused, size_t size, Comm::Flag errflag) STUB
-void ClientSocketContext::keepaliveNextRequest() STUB
 void ClientSocketContext::pullData() STUB
 int64_t ClientSocketContext::getNextRangeOffset() const STUB_RETVAL(0)
 bool ClientSocketContext::canPackMoreRanges() const STUB_RETVAL(false)
-- 
2.47.2