Require strict JobWait::start/finish pairing

I hesitated removing !fooWait asserts that many fooWait.start() callers
had at the beginning of their start-waiting methods. The earlier we
assert, the better, and those early asserts looked correct. However,
there is also a danger in asserting too early or for now good reason (as
the code gets refactored) and most of the code separating current
asserts and fooWait.start() calls are relatively simple job
configuration manipulations that are unlikely to significantly alter the
caller state or confuse developers during assertion triage. Keeping
these early asserts consistent across future changes would also be
difficult. Thus, I decided that it is better to remove those asserts
while relying on JobWait asserts, now built into the JobWait class.

I considered making JobWait::finish() forgiving, conditional on being in
waiting() state but succumbed to pressures to address concerns about
possible message corruption or incorrect routing decisions due to
mismatched/out-of-order callbacks.

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 60b3a949ec3e5eabab20da2c4d161d5c17b78db4..0b275a852c2519fb016971f8d58d134e9290db8d 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -874,8 +874,6 @@ FwdState::noteConnection(HappyConnOpener::Answer &answer)
 void
 FwdState::establishTunnelThruProxy(const Comm::ConnectionPointer &conn)
 {
-    assert(!httpConnectWait);
-
     AsyncCall::Pointer callback = asyncCall(17,4,
                                             "FwdState::tunnelEstablishmentDone",
                                             Http::Tunneler::CbDialer<FwdState>(&FwdState::tunnelEstablishmentDone, this));
@@ -968,7 +966,6 @@ FwdState::secureConnectionToPeerIfNeeded(const Comm::ConnectionPointer &conn)
 void
 FwdState::secureConnectionToPeer(const Comm::ConnectionPointer &conn)
 {
-    assert(!encryptionWait);
     HttpRequest::Pointer requestPointer = request;
     AsyncCall::Pointer callback = asyncCall(17,4,
                                             "FwdState::ConnectedToPeer",
@@ -1076,7 +1073,6 @@ FwdState::connectStart()
     Must(!request->pinnedConnection());
 
     assert(!destinations->empty());
-    assert(!tcpConnWait);
     assert(!usingDestination());
 
     // Ditch error page if it was created before.

diff --git a/src/PeerPoolMgr.cc b/src/PeerPoolMgr.cc
index a423032669a7e8fa607d9c9347df987cda6477c9..a46d8080850f546856e2da503fa2d5080cc5dc5a 100644 (file)
--- a/src/PeerPoolMgr.cc
+++ b/src/PeerPoolMgr.cc
@@ -90,7 +90,6 @@ PeerPoolMgr::doneAll() const
 void
 PeerPoolMgr::handleOpenedConnection(const CommConnectCbParams &params)
 {
-    assert(connWait);
     connWait.finish();
 
     if (!validPeer()) {
@@ -146,7 +145,6 @@ PeerPoolMgr::pushNewConnection(const Comm::ConnectionPointer &conn)
 void
 PeerPoolMgr::handleSecuredPeer(Security::EncryptorAnswer &answer)
 {
-    assert(encryptionWait);
     encryptionWait.finish();
 
     if (closer != NULL) {

diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc
index 5dadc2417f63b68632398a05fe4063ff98a13bc0..4ae30fc599f6fae5cf28131efb6fdf13ac85be08 100644 (file)
--- a/src/adaptation/icap/Xaction.cc
+++ b/src/adaptation/icap/Xaction.cc
@@ -260,7 +260,6 @@ void Adaptation::Icap::Xaction::closeConnection()
 /// called when the connection attempt to an ICAP service completes (successfully or not)
 void Adaptation::Icap::Xaction::noteCommConnected(const CommConnectCbParams &io)
 {
-    assert(connWait);
     connWait.finish();
 
     if (io.flag == Comm::TIMEOUT) {
@@ -725,7 +724,6 @@ Ssl::IcapPeerConnector::noteNegotiationDone(ErrorState *error)
 void
 Adaptation::Icap::Xaction::handleSecuredPeer(Security::EncryptorAnswer &answer)
 {
-    Must(encryptionWait);
     encryptionWait.finish();
 
     if (closer != NULL) {

diff --git a/src/base/AsyncJobCalls.h b/src/base/AsyncJobCalls.h
index 8822411e2211ff3b5c31a3870be31acedd93dbaa..45bce77db11cee56aae91308c1ced1d7e7b3ce65 100644 (file)
--- a/src/base/AsyncJobCalls.h
+++ b/src/base/AsyncJobCalls.h
@@ -204,10 +204,9 @@ public:
     /// starts waiting for the given job to call the given callback
     void start(JobPointer, AsyncCall::Pointer);
 
-    /// ends wait (if any) after receiving the call back
+    /// ends wait (after receiving the call back)
     /// forgets the job which is likely to be gone by now
-    /// does nothing if we are not waiting (TODO: assert that we are waiting)
-    void finish() { clear(); }
+    void finish();
 
     /// aborts wait (if any) before receiving the call back
     /// does nothing if we are not waiting
@@ -240,12 +239,27 @@ JobWait<Job>::start(const JobPointer aJob, const AsyncCall::Pointer aCall)
     assert(aCall);
     assert(aJob.valid());
 
-    // TODO: Should we also assert !callback_ and !job_?
+    // "Double" waiting state leads to conflicting/mismatching callbacks
+    // detailed in finish(). Detect that bug ASAP.
+    assert(!waiting());
 
+    assert(!callback_);
+    assert(!job_);
     callback_ = aCall;
     job_ = aJob;
 }
 
+template<class Job>
+void
+JobWait<Job>::finish()
+{
+    // Unexpected callbacks might result in disasters like secrets exposure,
+    // data corruption, or expensive message routing mistakes when the callback
+    // info is applied to the wrong message part or acted upon prematurely.
+    assert(waiting());
+    clear();
+}
+
 template<class Job>
 void
 JobWait<Job>::cancel(const char *reason)

diff --git a/src/clients/FtpGateway.cc b/src/clients/FtpGateway.cc
index e39d50fbf4dd3598b28ed18c1f570687d682876c..c32dfbd9b3cebed1254e21f8cc354c78934a70d1 100644 (file)
--- a/src/clients/FtpGateway.cc
+++ b/src/clients/FtpGateway.cc
@@ -1720,7 +1720,6 @@ void
 Ftp::Gateway::dataChannelConnected(const CommConnectCbParams &io)
 {
     debugs(9, 3, HERE);
-    assert(dataConnWait);
     dataConnWait.finish();
 
     if (io.flag != Comm::OK) {

diff --git a/src/clients/FtpRelay.cc b/src/clients/FtpRelay.cc
index 1fa5902afec9ef6ce2f2df4fe0035f2284cbe495..11cfd14c5996be3944b1bf604c4cc5283b3411b3 100644 (file)
--- a/src/clients/FtpRelay.cc
+++ b/src/clients/FtpRelay.cc
@@ -732,7 +732,6 @@ void
 Ftp::Relay::dataChannelConnected(const CommConnectCbParams &io)
 {
     debugs(9, 3, status());
-    assert(dataConnWait);
     dataConnWait.finish();
 
     if (io.flag != Comm::OK) {

diff --git a/src/log/TcpLogger.cc b/src/log/TcpLogger.cc
index 474ce4189e402c7e05ee4aeb1369c0cb6b3e9445..6ce081eca58d1410f097fb7b6e736a5e00ed22aa 100644 (file)
--- a/src/log/TcpLogger.cc
+++ b/src/log/TcpLogger.cc
@@ -265,7 +265,6 @@ Log::TcpLogger::doConnect()
 void
 Log::TcpLogger::connectDone(const CommConnectCbParams &params)
 {
-       assert(connWait);
        connWait.finish();
 
     if (params.flag != Comm::OK) {

diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc
index 89bb1bdd48b98e6d6ad91be030b928b194b8513a..3a6794d44e0b2edee14bcbee264ed52fd1581335 100644 (file)
--- a/src/security/PeerConnector.cc
+++ b/src/security/PeerConnector.cc
@@ -667,7 +667,6 @@ Security::PeerConnector::startCertDownloading(SBuf &url)
 
     const Downloader *csd = (request ? dynamic_cast<const Downloader*>(request->downloader.valid()) : nullptr);
     Downloader *dl = new Downloader(url, certCallback, XactionInitiator::initCertFetcher, csd ? csd->nestedLevel() + 1 : 1);
-    assert(!certDownloadWait);
     certDownloadWait.start(dl, certCallback);
     AsyncJob::Start(dl);
 }
@@ -675,7 +674,6 @@ Security::PeerConnector::startCertDownloading(SBuf &url)
 void
 Security::PeerConnector::certDownloadingDone(SBuf &obj, int downloadStatus)
 {
-    assert(certDownloadWait);
     certDownloadWait.finish();
 
     ++certsDownloads;

diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc
index f68ff907e642d4903c182d83d26c7a0c3a0f96bc..5fb97398481cb0f84f73a2b5bf369bf816a6fbb3 100644 (file)
--- a/src/servers/FtpServer.cc
+++ b/src/servers/FtpServer.cc
@@ -1701,7 +1701,6 @@ Ftp::Server::checkDataConnPost() const
 void
 Ftp::Server::connectedForData(const CommConnectCbParams &params)
 {
-    assert(dataConnWait);
     dataConnWait.finish();
 
     if (params.flag != Comm::OK) {

diff --git a/src/tunnel.cc b/src/tunnel.cc
index 4a9d996f80e5e6fac2edf6f8d8229ca74d442040..cd9ec30b11dfdf60184ed58b27944c5c31aec430 100644 (file)
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -904,7 +904,10 @@ TunnelStateData::copyServerBytes()
 static void
 tunnelStartShoveling(TunnelStateData *tunnelState)
 {
+    assert(!tunnelState->tcpConnWait);
+    assert(!tunnelState->encryptionWait);
     assert(!tunnelState->httpConnectWait);
+
     assert(tunnelState->server.conn);
     AsyncCall::Pointer timeoutCall = commCbCall(5, 4, "tunnelTimeout",
                                      CommTimeoutCbPtrFun(tunnelTimeout, tunnelState));
@@ -1162,7 +1165,6 @@ TunnelStateData::connectToPeer(const Comm::ConnectionPointer &conn)
 void
 TunnelStateData::secureConnectionToPeer(const Comm::ConnectionPointer &conn)
 {
-    assert(!encryptionWait);
     AsyncCall::Pointer callback = asyncCall(5,4, "TunnelStateData::noteSecurityPeerConnectorAnswer",
                                             MyAnswerDialer(&TunnelStateData::noteSecurityPeerConnectorAnswer, this));
     const auto connector = new Security::BlindPeerConnector(request, conn, callback, al);
@@ -1233,8 +1235,6 @@ TunnelStateData::connectedToPeer(const Comm::ConnectionPointer &conn)
 void
 TunnelStateData::establishTunnelThruProxy(const Comm::ConnectionPointer &conn)
 {
-    assert(!httpConnectWait);
-
     AsyncCall::Pointer callback = asyncCall(5,4,
                                             "TunnelStateData::tunnelEstablishmentDone",
                                              Http::Tunneler::CbDialer<TunnelStateData>(&TunnelStateData::tunnelEstablishmentDone, this));
@@ -1361,7 +1361,6 @@ TunnelStateData::startConnecting()
         request->hier.startPeerClock();
 
     assert(!destinations->empty());
-    assert(!tcpConnWait);
     assert(!usingDestination());
     AsyncCall::Pointer callback = asyncCall(17, 5, "TunnelStateData::noteConnection", HappyConnOpener::CbDialer<TunnelStateData>(&TunnelStateData::noteConnection, this));
     const auto cs = new HappyConnOpener(destinations, callback, request, startTime, 0, al);