From: Alex Rousskov Date: Thu, 15 Jul 2021 20:49:36 +0000 (-0400) Subject: fixup: Moved Job_type-agnostic JobWait code to a .cc file X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=be9f295b6a528031c4174da3acea1a43121521d3;p=thirdparty%2Fsquid.git fixup: Moved Job_type-agnostic JobWait code to a .cc file It is possible to make most JobWait users template-free. I have done that and reverted those changes because they: * Decrease code readability: The wait member declaration no longer makes it clear what job kind that wait member is waiting for/dedicated to. * Decrease JobWait safety (a user may give the wrong job to the wait.start() call) * Require strange/ugly tricks to make sure that users of the templated version do not call the non-templated start() which does not set the type-aware JobWaitAndCommunicate job_ member. A couple of other, very minor JobWait polishing touches. --- diff --git a/src/base/AsyncJobCalls.cc b/src/base/AsyncJobCalls.cc new file mode 100644 index 0000000000..2e88a97521 --- /dev/null +++ b/src/base/AsyncJobCalls.cc @@ -0,0 +1,79 @@ +/* + * Copyright (C) 1996-2020 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#include "squid.h" +#include "base/AsyncJobCalls.h" +#include "Debug.h" + +#include +#include + +JobWaitBase::JobWaitBase() = default; + +JobWaitBase::~JobWaitBase() +{ + cancel("owner gone"); +} + +void +JobWaitBase::start_(const AsyncJob::Pointer aJob, const AsyncCall::Pointer aCall) +{ + // Invariant: The wait will be over. We cannot guarantee that the job will + // call the callback, of course, but we can check these prerequisites. + assert(aCall); + assert(aJob.valid()); + + // "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; +} + +void +JobWaitBase::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(); +} + +void +JobWaitBase::cancel(const char *reason) +{ + if (callback_) { + callback_->cancel(reason); + + // Instead of AsyncJob, the class parameter could be Job. That would + // avoid runtime child-to-parent CbcPointer conversion overheads, but + // complicate support for Jobs with virtual AsyncJob bases (GCC error: + // "pointer to member conversion via virtual base AsyncJob") and also + // cache-log "Job::handleStopRequest()" with a non-existent class name. + CallJobHere(callback_->debugSection, callback_->debugLevel, job_, AsyncJob, handleStopRequest); + + clear(); + } +} + +void +JobWaitBase::print(std::ostream &os) const +{ + // use a backarrow to emphasize that this is a callback: call24<-job6 + if (callback_) + os << callback_->id << "<-"; + if (const auto rawJob = job_.get()) + os << rawJob->id; + else + os << job_; // raw pointer of a gone job may still be useful for triage +} + diff --git a/src/base/AsyncJobCalls.h b/src/base/AsyncJobCalls.h index 4e1f303545..ec9a968ae7 100644 --- a/src/base/AsyncJobCalls.h +++ b/src/base/AsyncJobCalls.h @@ -13,6 +13,8 @@ #include "base/CbcPointer.h" #include "Debug.h" +#include + /** \ingroup AsyncJobAPI * This is a base class for all job call dialers. It does all the job @@ -182,18 +184,16 @@ JobDialer::dial(AsyncCall &call) job->callEnd(); // may delete job } -/// manages waiting for an AsyncJob callback -template -class JobWait +/// Manages waiting for an AsyncJob callback. Use type-safe JobWait instead. +/// This base class does not contain code specific to the actual Job type. +class JobWaitBase { public: - typedef CbcPointer JobPointer; - - JobWait() = default; - ~JobWait() { cancel("owner gone"); } + JobWaitBase(); + ~JobWaitBase(); /// no copying of any kind: each waiting context needs a dedicated AsyncCall - JobWait(JobWait &&) = delete; + JobWaitBase(JobWaitBase &&) = delete; explicit operator bool() const { return waiting(); } @@ -201,9 +201,6 @@ public: /// the job itself may be gone even if this returns true bool waiting() const { return bool(callback_); } - /// starts waiting for the given job to call the given callback - void start(JobPointer, AsyncCall::Pointer); - /// ends wait (after receiving the call back) /// forgets the job which is likely to be gone by now void finish(); @@ -212,91 +209,52 @@ public: /// does nothing if we are not waiting void cancel(const char *reason); - /// \returns a cbdata pointer to the job we are waiting for (or nil) - /// the returned CbcPointer may be falsy, even if we are still waiting() - const JobPointer &job() const { return job_; } - /// summarizes what we are waiting for (for debugging) - std::ostream &print(std::ostream &) const; + void print(std::ostream &) const; + +protected: + /// starts waiting for the given job to call the given callback + void start_(AsyncJob::Pointer, AsyncCall::Pointer); private: /// the common part of finish() and cancel() - void clear() { callback_ = nullptr; job_.clear(); } + void clear() { job_.clear(); callback_ = nullptr; } /// the job that we are waiting to call us back (or nil) - JobPointer job_; + AsyncJob::Pointer job_; /// the call we are waiting for the job_ to make (or nil) AsyncCall::Pointer callback_; }; -template -void -JobWait::start(const JobPointer aJob, const AsyncCall::Pointer aCall) -{ - // Invariant: The wait will be over. We cannot guarantee that the job will - // call the callback, of course, but we can check these prerequisites. - assert(aCall); - assert(aJob.valid()); - - // "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 -void -JobWait::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 -void -JobWait::cancel(const char *reason) +/// Manages waiting for an AsyncJob callback. +/// Completes JobWaitBase by providing Job type-specific members. +template +class JobWait: public JobWaitBase { - if (callback_) { - callback_->cancel(reason); - - // Instead of AsyncJob, the class parameter could be Job. That would - // avoid runtime child-to-parent CbcPointer conversion overheads, but - // complicate support for Jobs with virtual AsyncJob bases (GCC error: - // "pointer to member conversion via virtual base AsyncJob") and also - // cache-log "Job::handleStopRequest()" with a non-existent class name. - CallJobHere(callback_->debugSection, callback_->debugLevel, job_, AsyncJob, handleStopRequest); +public: + typedef CbcPointer JobPointer; - clear(); + /// starts waiting for the given job to call the given callback + void start(const JobPointer &aJob, const AsyncCall::Pointer &aCallback) { + start_(aJob, aCallback); + typedJob_ = aJob; } -} -template -std::ostream & -JobWait::print(std::ostream &os) const -{ - // use a backarrow to emphasize that this is a callback: call24<-job6 - if (callback_) - os << callback_->id << "<-"; - if (const auto rawJob = job_.get()) - os << rawJob->id; - else - os << job_; // raw pointer of a gone job may still be useful for triage - return os; -} + /// \returns a cbdata pointer to the job we are waiting for (or nil) + /// the returned pointer may be falsy, even if we are still waiting() + JobPointer job() const { return waiting() ? typedJob_ : nullptr; } + +private: + /// nearly duplicates JobWaitBase::typedJob_ but exposes the actual job type + JobPointer typedJob_; +}; -template inline -std::ostream &operator <<(std::ostream &os, const JobWait &cbPointer) +std::ostream &operator <<(std::ostream &os, const JobWaitBase &wait) { - return cbPointer.print(os); + wait.print(os); + return os; } #endif /* SQUID_ASYNCJOBCALLS_H */ diff --git a/src/base/Makefile.am b/src/base/Makefile.am index 4792522f8d..f1934d6b34 100644 --- a/src/base/Makefile.am +++ b/src/base/Makefile.am @@ -18,6 +18,7 @@ libbase_la_SOURCES = \ AsyncCbdataCalls.h \ AsyncJob.cc \ AsyncJob.h \ + AsyncJobCalls.cc \ AsyncJobCalls.h \ ByteCounter.h \ CbDataList.h \