From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Wed, 14 Jul 2021 22:37:19 +0000 (-0400)
Subject: fixup: Removed excessive branch-added wait cancellations
X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=c229ea4d4393c4666eea46ae14beb412f8cb6cf6;p=thirdparty%2Fsquid.git

fixup: Removed excessive branch-added wait cancellations

The swanSong() method does not need to remember to cancel waits. That
will happen automatically upon imminent job destruction.

Adaptation::Icap::Xaction::noteCommClosed() does not need to remember to
cancel encryption wait because the encryptor will see the same
connection closure notification. The Xaction object should not even have
the connection closure handler while another job is negotiating TLS, but
that is one of the problems I hope we do not have to fix now.

Also added nasty XXXs. This old code needs some love, but fixing these
old problems is far from trivial and is outside this branch scope.

Also reduced master diff a little.
---

diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc
index 3e3de02188..d0da312625 100644
--- a/src/adaptation/icap/Xaction.cc
+++ b/src/adaptation/icap/Xaction.cc
@@ -197,6 +197,7 @@ Adaptation::Icap::Xaction::dnsLookupDone(const ipcache_addrs *ia)
         return;
     }
 
+    // XXX: Do not save this half-baked connection. Just pass it to ConnOpener.
     connection = new Comm::Connection;
     connection->remote = ia->current();
     connection->remote.port(s.cfg().port);
@@ -279,6 +280,9 @@ Adaptation::Icap::Xaction::successfullyConnected()
 {
     assert(Comm::IsConnOpen(connection));
 
+    // XXX: We should not set timeout and closure handlers here if we are going
+    // to negotiate TLS. Only Ssl::IcapPeerConnector should own the connection.
+
     typedef CommCbMemFunT<Adaptation::Icap::Xaction, CommTimeoutCbParams> TimeoutDialer;
     AsyncCall::Pointer timeoutCall =  asyncCall(93, 5, "Adaptation::Icap::Xaction::noteCommTimedout",
                                       TimeoutDialer(this,&Adaptation::Icap::Xaction::noteCommTimedout));
@@ -374,7 +378,6 @@ void Adaptation::Icap::Xaction::handleCommTimedout()
 // unexpected connection close while talking to the ICAP service
 void Adaptation::Icap::Xaction::noteCommClosed(const CommCloseCbParams &)
 {
-    encryptionWait.cancel("Connection closed before SSL negotiation finished");
     closer = NULL;
     handleCommClosed();
 }
@@ -586,12 +589,9 @@ void Adaptation::Icap::Xaction::swanSong()
 {
     // kids should sing first and then call the parent method.
     if (connWait) {
-        connWait.cancel("Icap::Xaction::swanSong");
         service().noteConnectionFailed("abort");
     }
 
-    encryptionWait.cancel("Icap::Xaction::swanSong");
-
     closeConnection(); // TODO: rename because we do not always close
 
     readBuf.clear();
@@ -750,3 +750,4 @@ Adaptation::Icap::Xaction::handleSecuredPeer(Security::EncryptorAnswer &answer)
 
     handleCommConnected();
 }
+