]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Improved AsyncJob::Start() API and reduced code duplication
authorAlex Rousskov <rousskov@measurement-factory.com>
Fri, 6 Aug 2021 20:20:33 +0000 (16:20 -0400)
committerAlex Rousskov <rousskov@measurement-factory.com>
Fri, 6 Aug 2021 20:22:32 +0000 (16:22 -0400)
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).

17 files changed:
src/FwdState.cc
src/HappyConnOpener.cc
src/PeerPoolMgr.cc
src/adaptation/icap/Xaction.cc
src/base/AsyncJob.cc
src/base/AsyncJob.h
src/base/JobWait.cc
src/clients/FtpClient.cc
src/clients/FtpGateway.cc
src/clients/FtpRelay.cc
src/clients/forward.h
src/fs/rock/RockRebuild.cc
src/ident/Ident.cc
src/log/TcpLogger.cc
src/security/PeerConnector.cc
src/servers/FtpServer.cc
src/tunnel.cc

index 0d328291d7a35407aec44d4d74c0a0cf7a7bbd76..df33e18b4c2db8974c32dd603d32388b02f30019 100644 (file)
@@ -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
index 94eef88104d4ac5b877cfb1f1510afa50efb5d4a..832e660d00b6db505f46e6e473963b5e7369fa1c 100644 (file)
@@ -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
index 5e8d9986d1901fe82efa7271ba5a015024767f96..4895fdd19b2a0126fbd483ed4882a62d30952fde 100644 (file)
@@ -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
index 3a6bc94df894762aa4611fd8eacdba7704752361..5f340d12cf135b1d52ad9da73c70fa8988b02c79 100644 (file)
@@ -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;
     }
 
index 16d444e01792a6983b4d5403b3c6a5812e06506f..267f43bf9d7edcfeed99de57a2f7481b61ffe66b 100644 (file)
 
 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) :
index fe3e26d74fc57a96132413a74f296ced3285101c..c21b48989a8098e778a5777ab6a243c14090efbc 100644 (file)
@@ -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.
index cc6b0c5d5f667ad2732a18738b275ccad4c771e2..ba68324157193cf9880aa11e320d3c945fe40547 100644 (file)
@@ -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
index 578a3043ffdf0c424cdf10987d8087270e172db3..9830af8cb9d41ae4b5d609d47ad2e64b634b275f 100644 (file)
@@ -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
index c32dfbd9b3cebed1254e21f8cc354c78934a70d1..3e63d261ad4b72c95c230232be3711223841391f 100644 (file)
@@ -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));
 }
 
index 11cfd14c5996be3944b1bf604c4cc5283b3411b3..bb6429d59bfff1bee3853c12532964eab06b9816 100644 (file)
@@ -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));
 }
 
index ca9faf53528b194866ac176b13b67a3cf6bf00f3..250ae89eacdead51726a676a6030a343e4579d81 100644 (file)
@@ -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 '/'
index 2d41da3d0a282b98182b252854cfe9bacbb27316..7dcbe5240cbeba7b8746c96e2a29703f78eef523 100644 (file)
@@ -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;
 }
 
index 5f9e574ef5c3e7060cca2f5b3e0905d6d10ee4eb..4ca9926f9f5bbf22f7cabdb6519e81f1db5713a7 100644 (file)
@@ -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
index ee6fa317154aa5d7dbfcfa078b26739094038738..c16f6d42a68fb2c0dc85c35e37ec9d323bdf8064 100644 (file)
@@ -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
index d0cfeb81368b33f5230e1d7631a53efc5ab28667..0f572f9bf0868654e6e59e17013c76dfab71e500 100644 (file)
@@ -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
index acafb3fbbb97086e3deea650ed674c8887eeef29..a7643bdf4634a6e8f8bb7b6068cb9262f62148f3 100644 (file)
@@ -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.
index aa8daea08c2a4a2841124b9afb7b4c7646da25fb..3324b020ffc409cf256d91d9a9c9a6b8cfdda129 100644 (file)
@@ -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