From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Fri, 6 Aug 2021 20:20:33 +0000 (-0400)
Subject: Improved AsyncJob::Start() API and reduced code duplication
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=9cc73a91b3258b69e66c36637cbca2091c05c032;p=thirdparty%2Fsquid.git

Improved AsyncJob::Start() API and reduced code duplication

Every JobWait::start() call was followed by AsyncJob::Start() call. The
relationship is natural. JobWait::start() should call Start() directly.

AsyncJob::Start() used to return a job pointer, but that was a bad idea:

* It implies that a failed Start() will return a nil pointer, and that
  the caller should check the result. Neither is true.

* It encourages callers to dereference the returned pointer to adjust
  the job. That technically works (today) but violates the rules of
  communicating with an async job. The Start() method is the boundary
  after which the job is deemed asynchronous.

Also removed old "and wait for..." comments because the code itself
became clear enough, and those comments were becoming increasingly stale
(because they duplicated the callback names above them).
---

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 0d328291d7..df33e18b4c 100644
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -896,8 +896,6 @@ FwdState::establishTunnelThruProxy(const Comm::ConnectionPointer &conn)
         tunneler->setDelayId(entry->mem_obj->mostBytesAllowed());
 #endif
     httpConnectWait.start(tunneler, callback);
-    AsyncJob::Start(tunneler);
-    // and wait for the tunnelEstablishmentDone() call
 }
 
 /// resumes operations after the (possibly failed) HTTP CONNECT exchange
@@ -985,7 +983,6 @@ FwdState::secureConnectionToPeer(const Comm::ConnectionPointer &conn)
         connector = new Security::BlindPeerConnector(requestPointer, conn, callback, al, sslNegotiationTimeout);
     connector->noteFwdPconnUse = true;
     encryptionWait.start(connector, callback);
-    AsyncJob::Start(connector); // will call our callback
 }
 
 /// called when all negotiations with the TLS-speaking peer have been completed
@@ -1104,7 +1101,6 @@ FwdState::connectStart()
     cs->allowPersistent(pconnRace != raceHappened);
     destinations->notificationPending = true; // start() is async
     tcpConnWait.start(cs, callback);
-    AsyncJob::Start(cs);
 }
 
 /// send request on an existing connection dedicated to the requesting client
diff --git a/src/HappyConnOpener.cc b/src/HappyConnOpener.cc
index 94eef88104..832e660d00 100644
--- a/src/HappyConnOpener.cc
+++ b/src/HappyConnOpener.cc
@@ -580,8 +580,6 @@ HappyConnOpener::openFreshConnection(Attempt &attempt, PeerConnectionPointer &de
 
     attempt.path = dest; // but not the being-opened conn!
     attempt.connWait.start(cs, callConnect);
-
-    AsyncJob::Start(cs);
 }
 
 /// Comm::ConnOpener callback for the prime connection attempt
diff --git a/src/PeerPoolMgr.cc b/src/PeerPoolMgr.cc
index 5e8d9986d1..4895fdd19b 100644
--- a/src/PeerPoolMgr.cc
+++ b/src/PeerPoolMgr.cc
@@ -118,7 +118,6 @@ PeerPoolMgr::handleOpenedConnection(const CommConnectCbParams &params)
         const int timeLeft = positiveTimeout(peerTimeout - timeUsed);
         const auto connector = new Security::BlindPeerConnector(request, params.conn, callback, nullptr, timeLeft);
         encryptionWait.start(connector, callback);
-        AsyncJob::Start(connector); // will call our callback
         return;
     }
 
@@ -202,7 +201,6 @@ PeerPoolMgr::openNewConnection()
     AsyncCall::Pointer callback = JobCallback(48, 5, Dialer, this, PeerPoolMgr::handleOpenedConnection);
     const auto cs = new Comm::ConnOpener(conn, callback, ctimeout);
     connWait.start(cs, callback);
-    AsyncJob::Start(cs);
 }
 
 void
diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc
index 3a6bc94df8..5f340d12cf 100644
--- a/src/adaptation/icap/Xaction.cc
+++ b/src/adaptation/icap/Xaction.cc
@@ -200,7 +200,6 @@ Adaptation::Icap::Xaction::dnsLookupDone(const ipcache_addrs *ia)
     const auto cs = new Comm::ConnOpener(conn, callback, TheConfig.connect_timeout(service().cfg().bypass));
     cs->setHost(s.cfg().host.termedBuf());
     connWait.start(cs, callback);
-    AsyncJob::Start(cs);
 }
 
 /*
@@ -284,7 +283,6 @@ Adaptation::Icap::Xaction::useTransportConnection(const Comm::ConnectionPointer
         const auto sslConnector = new Ssl::IcapPeerConnector(theService, conn, callback, masterLogEntry(), TheConfig.connect_timeout(service().cfg().bypass));
 
         encryptionWait.start(sslConnector, callback);
-        AsyncJob::Start(sslConnector); // will call our callback
         return;
     }
 
diff --git a/src/base/AsyncJob.cc b/src/base/AsyncJob.cc
index 16d444e017..267f43bf9d 100644
--- a/src/base/AsyncJob.cc
+++ b/src/base/AsyncJob.cc
@@ -20,12 +20,11 @@
 
 InstanceIdDefinitions(AsyncJob, "job");
 
-AsyncJob::Pointer AsyncJob::Start(AsyncJob *j)
+void
+AsyncJob::Start(const Pointer &job)
 {
-    AsyncJob::Pointer job(j);
     CallJobHere(93, 5, job, AsyncJob, start);
     job->started_ = true; // it is the attempt that counts
-    return job;
 }
 
 AsyncJob::AsyncJob(const char *aTypeName) :
diff --git a/src/base/AsyncJob.h b/src/base/AsyncJob.h
index fe3e26d74f..c21b48989a 100644
--- a/src/base/AsyncJob.h
+++ b/src/base/AsyncJob.h
@@ -36,8 +36,13 @@ public:
 public:
     AsyncJob(const char *aTypeName);
 
-    /// starts a freshly created job (i.e., makes the job asynchronous)
-    static Pointer Start(AsyncJob *job);
+    /// Promises to start the configured job (eventually). The job is deemed to
+    /// be running asynchronously beyond this point, so the caller should only
+    /// access the job object via AsyncCalls rather than directly.
+    ///
+    /// swanSong() is only called for jobs for which this method has returned
+    /// successfully (i.e. without throwing).
+    static void Start(const Pointer &job);
 
 protected:
     // XXX: temporary method to replace "delete this" in jobs-in-transition.
diff --git a/src/base/JobWait.cc b/src/base/JobWait.cc
index cc6b0c5d5f..ba68324157 100644
--- a/src/base/JobWait.cc
+++ b/src/base/JobWait.cc
@@ -36,6 +36,8 @@ JobWaitBase::start_(const AsyncJob::Pointer aJob, const AsyncCall::Pointer aCall
     assert(!job_);
     callback_ = aCall;
     job_ = aJob;
+
+    AsyncJob::Start(job_.get());
 }
 
 void
diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc
index 578a3043ff..9830af8cb9 100644
--- a/src/clients/FtpClient.cc
+++ b/src/clients/FtpClient.cc
@@ -769,7 +769,6 @@ Ftp::Client::connectDataChannel()
     const auto cs = new Comm::ConnOpener(conn, callback, Config.Timeout.connect);
     cs->setHost(data.host);
     dataConnWait.start(cs, callback);
-    AsyncJob::Start(cs);
 }
 
 bool
diff --git a/src/clients/FtpGateway.cc b/src/clients/FtpGateway.cc
index c32dfbd9b3..3e63d261ad 100644
--- a/src/clients/FtpGateway.cc
+++ b/src/clients/FtpGateway.cc
@@ -2724,9 +2724,9 @@ Ftp::Gateway::mayReadVirginReplyBody() const
     return !doneWithServer();
 }
 
-AsyncJob::Pointer
+void
 Ftp::StartGateway(FwdState *const fwdState)
 {
-    return AsyncJob::Start(new Ftp::Gateway(fwdState));
+    AsyncJob::Start(new Ftp::Gateway(fwdState));
 }
 
diff --git a/src/clients/FtpRelay.cc b/src/clients/FtpRelay.cc
index 11cfd14c59..bb6429d59b 100644
--- a/src/clients/FtpRelay.cc
+++ b/src/clients/FtpRelay.cc
@@ -797,9 +797,9 @@ Ftp::Relay::HandleStoreAbort(Relay *ftpClient)
         ftpClient->dataComplete();
 }
 
-AsyncJob::Pointer
+void
 Ftp::StartRelay(FwdState *const fwdState)
 {
-    return AsyncJob::Start(new Ftp::Relay(fwdState));
+    AsyncJob::Start(new Ftp::Relay(fwdState));
 }
 
diff --git a/src/clients/forward.h b/src/clients/forward.h
index ca9faf5352..250ae89eac 100644
--- a/src/clients/forward.h
+++ b/src/clients/forward.h
@@ -28,10 +28,10 @@ namespace Ftp
 {
 
 /// A new FTP Gateway job
-AsyncJobPointer StartGateway(FwdState *const fwdState);
+void StartGateway(FwdState *const fwdState);
 
 /// A new FTP Relay job
-AsyncJobPointer StartRelay(FwdState *const fwdState);
+void StartRelay(FwdState *const fwdState);
 
 /** Construct an URI with leading / in PATH portion for use by CWD command
  *  possibly others. FTP encodes absolute paths as beginning with '/'
diff --git a/src/fs/rock/RockRebuild.cc b/src/fs/rock/RockRebuild.cc
index 2d41da3d0a..7dcbe5240c 100644
--- a/src/fs/rock/RockRebuild.cc
+++ b/src/fs/rock/RockRebuild.cc
@@ -289,7 +289,7 @@ Rock::Rebuild::Start(SwapDir &dir)
         return false;
     }
 
-    Must(AsyncJob::Start(new Rebuild(&dir, stats)));
+    AsyncJob::Start(new Rebuild(&dir, stats));
     return true;
 }
 
diff --git a/src/ident/Ident.cc b/src/ident/Ident.cc
index 5f9e574ef5..4ca9926f9f 100644
--- a/src/ident/Ident.cc
+++ b/src/ident/Ident.cc
@@ -297,7 +297,6 @@ Ident::Start(const Comm::ConnectionPointer &conn, IDCB * callback, void *data)
     AsyncCall::Pointer call = commCbCall(30,3, "Ident::ConnectDone", CommConnectCbPtrFun(Ident::ConnectDone, state));
     const auto connOpener = new Comm::ConnOpener(identConn, call, Ident::TheConfig.timeout);
     state->connWait.start(connOpener, call);
-    AsyncJob::Start(connOpener);
 }
 
 void
diff --git a/src/log/TcpLogger.cc b/src/log/TcpLogger.cc
index ee6fa31715..c16f6d42a6 100644
--- a/src/log/TcpLogger.cc
+++ b/src/log/TcpLogger.cc
@@ -258,7 +258,6 @@ Log::TcpLogger::doConnect()
     AsyncCall::Pointer call = JobCallback(MY_DEBUG_SECTION, 5, Dialer, this, Log::TcpLogger::connectDone);
     const auto cs = new Comm::ConnOpener(futureConn, call, 2);
     connWait.start(cs, call);
-    AsyncJob::Start(cs);
 }
 
 /// Comm::ConnOpener callback
diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc
index d0cfeb8136..0f572f9bf0 100644
--- a/src/security/PeerConnector.cc
+++ b/src/security/PeerConnector.cc
@@ -656,7 +656,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);
     certDownloadWait.start(dl, certCallback);
-    AsyncJob::Start(dl);
 }
 
 void
diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc
index acafb3fbbb..a7643bdf46 100644
--- a/src/servers/FtpServer.cc
+++ b/src/servers/FtpServer.cc
@@ -1682,8 +1682,7 @@ Ftp::Server::checkDataConnPre()
     const auto cs = new Comm::ConnOpener(dataConn->cloneProfile(), callback,
                                          Config.Timeout.connect);
     dataConnWait.start(cs, callback);
-    AsyncJob::Start(cs);
-    return false; // ConnStateData::processFtpRequest waits handleConnectDone
+    return false;
 }
 
 /// Check that client data connection is ready for immediate I/O.
diff --git a/src/tunnel.cc b/src/tunnel.cc
index aa8daea08c..3324b020ff 100644
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -1170,7 +1170,6 @@ TunnelStateData::secureConnectionToPeer(const Comm::ConnectionPointer &conn)
                                             MyAnswerDialer(&TunnelStateData::noteSecurityPeerConnectorAnswer, this));
     const auto connector = new Security::BlindPeerConnector(request, conn, callback, al);
     encryptionWait.start(connector, callback);
-    AsyncJob::Start(connector); // will call our callback
 }
 
 /// starts a preparation step for an established connection; retries on failures
@@ -1236,8 +1235,6 @@ TunnelStateData::establishTunnelThruProxy(const Comm::ConnectionPointer &conn)
     tunneler->setDelayId(server.delayId);
 #endif
     httpConnectWait.start(tunneler, callback);
-    AsyncJob::Start(tunneler);
-    // and wait for the tunnelEstablishmentDone() call
 }
 
 void
@@ -1373,7 +1370,6 @@ TunnelStateData::startConnecting()
     cs->allowPersistent(false);
     destinations->notificationPending = true; // start() is async
     tcpConnWait.start(cs, callback);
-    AsyncJob::Start(cs);
 }
 
 /// send request on an existing connection dedicated to the requesting client