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().

diff --git a/src/client_side.cc b/src/client_side.cc
index 6bb345db10e2e83766bca579a50638cdf3534f6b..14087421daab7473df107b6df705eb87c00340d4 100644 (file)
--- 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 f31561661241a567d3ce41f004ed84a246cc514c..2e9c047d467f3f576f2b86d25b6fc37b6186aee0 100644 (file)
--- 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 5c740a1cd0d9e9861fa79d9d416f2a8cf544f267..bd1be2db2c3a2f46d045ae4033fe4671a191dfc0 100644 (file)
--- 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 e4f841aed74932d56d9c8ba5ded17f596b03c469..bf3a393694c111901cecdda673ae2668d09cc10e 100644 (file)
--- 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)