Improved Http::Tunneler::connection management

When sending a negative answer, we would set answer().conn to an open
connection, async-send the answer, and then hurry to close the
connection using our pointer to the shared Connection object. If
everything went according to the plan, the recipient would get a non-nil
but closed Connection object. Now, a negative answer simply means no
connection at all.

Also reduced Http::Tunneler exposure to a closed Connection object.

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 3de8e563f3840ad8804b893ca67cb83da2518f98..4371b8ea5d8cad6dbf66db94e2c86170f930391e 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -914,7 +914,7 @@ FwdState::tunnelEstablishmentDone(Http::TunnelerAnswer &answer)
 
     ErrorState *error = nullptr;
     if (!answer.positive()) {
-        Must(!Comm::IsConnOpen(answer.conn));
+        Must(!answer.conn);
         error = answer.squidError.get();
         Must(error);
         answer.squidError.clear(); // preserve error for fail()

diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc
index dd8a4ba947b59a06612d9b466880e327a9302523..dcd23cb6b78857e87b0d5f9a66da7dc84a68a10b 100644 (file)
--- a/src/clients/HttpTunneler.cc
+++ b/src/clients/HttpTunneler.cc
@@ -100,12 +100,12 @@ Http::Tunneler::start()
 void
 Http::Tunneler::handleConnectionClosure(const CommCloseCbParams &params)
 {
+    closer = nullptr;
     if (connection) {
+        countFailingConnection();
         connection->noteClosure();
-        // TODO: Properly get rid of connection here instead of keeping a closed
-        // connection object for peerConnectFailed(),noteUses() in bailWith().
+        connection = nullptr;
     }
-    closer = nullptr;
     bailWith(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al));
 }
 
@@ -365,56 +365,65 @@ Http::Tunneler::bailWith(ErrorState *error)
     Must(error);
     answer().squidError = error;
 
-    if (const auto p = connection->getPeer())
-        peerConnectFailed(p);
-
-    callBack();
-    disconnect();
-
-    // TODO: Close before callBack(); do not pretend to send an open connection.
-    if (Comm::IsConnOpen(connection)) {
-        if (noteFwdPconnUse)
-            fwdPconnPool->noteUses(fd_table[connection->fd].pconn.uses);
+    if (const auto failingConnection = connection) {
         // TODO: Reuse to-peer connections after a CONNECT error response.
-
-        assert(!closer); // the above disconnect() removes it
-        connection->close();
+        countFailingConnection();
+        disconnect();
+        failingConnection->close();
     }
-    connection = nullptr;
+
+    callBack();
 }
 
 void
 Http::Tunneler::sendSuccess()
 {
     assert(answer().positive());
-    callBack();
+    assert(Comm::IsConnOpen(connection));
+    answer().conn = connection;
     disconnect();
+    callBack();
+}
+
+
+void
+Http::Tunneler::countFailingConnection()
+{
+    assert(connection);
+    if (const auto p = connection->getPeer())
+        peerConnectFailed(p);
+    if (noteFwdPconnUse && connection->isOpen())
+        fwdPconnPool->noteUses(fd_table[connection->fd].pconn.uses);
 }
 
 void
 Http::Tunneler::disconnect()
 {
+    const auto stillOpen = Comm::IsConnOpen(connection);
+
     if (closer) {
-        comm_remove_close_handler(connection->fd, closer);
+        if (stillOpen)
+            comm_remove_close_handler(connection->fd, closer);
         closer = nullptr;
     }
 
     if (reader) {
-        if (Comm::IsConnOpen(connection))
+        if (stillOpen)
             Comm::ReadCancel(connection->fd, reader);
         reader = nullptr;
     }
 
-    if (Comm::IsConnOpen(connection))
+    if (stillOpen)
         commUnsetConnTimeout(connection);
+
+    connection = nullptr; // may still be open
 }
 
 void
 Http::Tunneler::callBack()
 {
-    debugs(83, 5, connection << status());
-    if (answer().positive())
-        answer().conn = connection;
+    debugs(83, 5, answer().conn << status());
+    assert(!connection); // returned inside answer() or gone
     auto cb = callback;
     callback = nullptr;
     ScheduleCallHere(cb);

diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h
index 08803eed999070e1d26fb493e72aa67076535d34..7eb393c2462916a7e0fe92c4fc042fc7bcb43b26 100644 (file)
--- a/src/clients/HttpTunneler.h
+++ b/src/clients/HttpTunneler.h
@@ -87,6 +87,7 @@ protected:
     void handleResponse(const bool eof);
     void bailOnResponseError(const char *error, HttpReply *);
 
+private:
     /// sends the given error to the initiator
     void bailWith(ErrorState*);
 
@@ -96,12 +97,14 @@ protected:
     /// a bailWith(), sendSuccess() helper: sends results to the initiator
     void callBack();
 
-    /// a bailWith(), sendSuccess() helper: stops monitoring the connection
+    /// stops monitoring the connection
     void disconnect();
 
+    /// updates connection usage history before the connection is closed
+    void countFailingConnection();
+
     TunnelerAnswer &answer();
 
-private:
     AsyncCall::Pointer writer; ///< called when the request has been written
     AsyncCall::Pointer reader; ///< called when the response should be read
     AsyncCall::Pointer closer; ///< called when the connection is being closed

diff --git a/src/tunnel.cc b/src/tunnel.cc
index 5d6630288fdc4d77ec388981c16f364db3284c6d..6d5eddd447acf25f42cf71003a15380c0f8a2713 100644 (file)
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -979,7 +979,7 @@ TunnelStateData::tunnelEstablishmentDone(Http::TunnelerAnswer &answer)
 
     if (!answer.positive()) {
         sawProblem = true;
-        Must(!Comm::IsConnOpen(answer.conn));
+        Must(!answer.conn);
     } else if (!Comm::IsConnOpen(answer.conn) || fd_table[answer.conn->fd].closing()) {
         sawProblem = true;
         closePendingConnection(answer.conn, "conn was closed while waiting for tunnelEstablishmentDone");