From: Eduard Bagdasaryan Date: Sat, 20 Jun 2020 16:58:14 +0000 (+0000) Subject: Preserve caller context in MemObject::abort callbacks (#671) X-Git-Tag: 4.15-20210522-snapshot~92 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=7e9f330d8b48ef2825bd08e53582522287e49c55;p=thirdparty%2Fsquid.git Preserve caller context in MemObject::abort callbacks (#671) Store abused time-based event API to deliver abort notifications, losing transaction context in the process. Further refactoring may crystallize the currently vague/obscured "store writer" concept. Such refactoring is outside this project scope but will benefit from these changes. --- diff --git a/src/FwdState.cc b/src/FwdState.cc index 6f32877b02..c468541b50 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -105,9 +105,8 @@ private: }; void -FwdState::abort(void* d) +FwdState::HandleStoreAbort(FwdState *fwd) { - FwdState* fwd = (FwdState*)d; Pointer tmp = fwd; // Grab a temporary pointer to keep the object alive during our scope. if (Comm::IsConnOpen(fwd->serverConnection())) { @@ -177,8 +176,10 @@ void FwdState::start(Pointer aSelf) // Ftp::Relay needs to preserve control connection on data aborts // so it registers its own abort handler that calls ours when needed. - if (!request->flags.ftpNative) - entry->registerAbort(FwdState::abort, this); + if (!request->flags.ftpNative) { + AsyncCall::Pointer call = asyncCall(17, 4, "FwdState::Abort", cbdataDialer(&FwdState::HandleStoreAbort, this)); + entry->registerAbortCallback(call); + } // just in case; should already be initialized to false request->flags.pinned = false; @@ -317,7 +318,7 @@ FwdState::~FwdState() delete err; - entry->unregisterAbort(); + entry->unregisterAbortCallback("FwdState object destructed"); entry->unlock("FwdState"); diff --git a/src/FwdState.h b/src/FwdState.h index f9cd6d37b2..0e052f0c33 100644 --- a/src/FwdState.h +++ b/src/FwdState.h @@ -172,7 +172,8 @@ public: HttpRequest *request; AccessLogEntryPointer al; ///< info for the future access.log entry - static void abort(void*); + /// called by Store if the entry is no longer usable + static void HandleStoreAbort(FwdState *); private: Pointer self; diff --git a/src/MemObject.h b/src/MemObject.h index 76686feb0b..c34b32007f 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -25,7 +25,6 @@ #endif typedef void STMCB (void *data, StoreIOBuffer wroteBuffer); -typedef void STABH(void *); class store_client; class PeerSelector; @@ -192,11 +191,8 @@ public: IRCB *ping_reply_callback; PeerSelector *ircb_data = nullptr; - struct abort_ { - abort_() { callback = nullptr; } - STABH *callback; - void *data = nullptr; - } abort; + /// used for notifying StoreEntry writers about 3rd-party initiated aborts + AsyncCall::Pointer abortCallback; RemovalPolicyNode repl; int id = 0; int64_t object_sz = -1; diff --git a/src/Store.h b/src/Store.h index 33209db903..7bf30d479b 100644 --- a/src/Store.h +++ b/src/Store.h @@ -150,11 +150,15 @@ public: void dump(int debug_lvl) const; void hashDelete(); void hashInsert(const cache_key *); - void registerAbort(STABH * cb, void *); + /// notify the StoreEntry writer of a 3rd-party initiated StoreEntry abort + void registerAbortCallback(const AsyncCall::Pointer &); void reset(); void setMemStatus(mem_status_t); bool timestampsSet(); - void unregisterAbort(); + /// Avoid notifying anybody about a 3rd-party initiated StoreEntry abort. + /// Calling this method does not cancel the already queued notification. + /// TODO: Refactor to represent the end of (shared) ownership by our writer. + void unregisterAbortCallback(const char *reason); void destroyMemObject(); int checkTooSmall(); diff --git a/src/clients/FtpRelay.cc b/src/clients/FtpRelay.cc index a141da5680..19967b3e62 100644 --- a/src/clients/FtpRelay.cc +++ b/src/clients/FtpRelay.cc @@ -10,6 +10,7 @@ #include "squid.h" #include "anyp/PortCfg.h" +#include "base/AsyncCbdataCalls.h" #include "client_side.h" #include "clients/forward.h" #include "clients/FtpClient.h" @@ -94,8 +95,8 @@ protected: /// Inform Ftp::Server that we are done if originWaitInProgress void stopOriginWait(int code); - - static void abort(void *d); // TODO: Capitalize this and FwdState::abort(). + /// called by Store if the entry is no longer usable + static void HandleStoreAbort(Relay *); bool forwardingCompleted; ///< completeForwarding() has been called @@ -161,12 +162,17 @@ Ftp::Relay::Relay(FwdState *const fwdState): // Nothing we can do at request creation time can mark the response as // uncachable, unfortunately. This prevents "found KEY_PRIVATE" WARNINGs. entry->releaseRequest(); - // TODO: Convert registerAbort() to use AsyncCall - entry->registerAbort(Ftp::Relay::abort, this); + AsyncCall::Pointer call = asyncCall(9, 4, "Ftp::Relay::Abort", cbdataDialer(&Relay::HandleStoreAbort, this)); + entry->registerAbortCallback(call); } Ftp::Relay::~Relay() { + entry->unregisterAbortCallback("Ftp::Relay object destructed"); + // Client, our parent, calls entry->unlock(). + // Client does not currently un/registerAbortCallback() because + // FwdState does that for other Client kids; \see FwdState::start(). + closeServer(); // TODO: move to clients/Client.cc? if (savedReply.message) wordlistDestroy(&savedReply.message); @@ -784,12 +790,9 @@ Ftp::Relay::stopOriginWait(int code) } void -Ftp::Relay::abort(void *d) +Ftp::Relay::HandleStoreAbort(Relay *ftpClient) { - Ftp::Relay *ftpClient = (Ftp::Relay *)d; debugs(9, 2, "Client Data connection closed!"); - if (!cbdataReferenceValid(ftpClient)) - return; if (Comm::IsConnOpen(ftpClient->data.conn)) ftpClient->dataComplete(); } diff --git a/src/mgr/StoreToCommWriter.cc b/src/mgr/StoreToCommWriter.cc index ed1206e29d..251440eb92 100644 --- a/src/mgr/StoreToCommWriter.cc +++ b/src/mgr/StoreToCommWriter.cc @@ -9,6 +9,7 @@ /* DEBUG: section 16 Cache Manager API */ #include "squid.h" +#include "base/AsyncCbdataCalls.h" #include "base/TextException.h" #include "comm/Connection.h" #include "comm/Write.h" @@ -57,7 +58,8 @@ Mgr::StoreToCommWriter::start() debugs(16, 6, HERE); Must(Comm::IsConnOpen(clientConnection)); Must(entry != NULL); - entry->registerAbort(&StoreToCommWriter::Abort, this); + AsyncCall::Pointer call = asyncCall(16, 4, "StoreToCommWriter::Abort", cbdataDialer(&StoreToCommWriter::HandleStoreAbort, this)); + entry->registerAbortCallback(call); sc = storeClientListAdd(entry, this); Must(sc != NULL); @@ -142,7 +144,7 @@ Mgr::StoreToCommWriter::swanSong() storeUnregister(sc, entry, this); sc = NULL; } - entry->unregisterAbort(); + entry->unregisterAbortCallback("StoreToCommWriter done"); entry->unlock("Mgr::StoreToCommWriter::swanSong"); entry = NULL; } @@ -158,9 +160,8 @@ Mgr::StoreToCommWriter::doneAll() const } void -Mgr::StoreToCommWriter::Abort(void* param) +Mgr::StoreToCommWriter::HandleStoreAbort(StoreToCommWriter *mgrWriter) { - StoreToCommWriter* mgrWriter = static_cast(param); if (Comm::IsConnOpen(mgrWriter->clientConnection)) mgrWriter->clientConnection->close(); } diff --git a/src/mgr/StoreToCommWriter.h b/src/mgr/StoreToCommWriter.h index 367d516549..84fb1cfe8d 100644 --- a/src/mgr/StoreToCommWriter.h +++ b/src/mgr/StoreToCommWriter.h @@ -45,7 +45,7 @@ protected: void noteStoreCopied(StoreIOBuffer ioBuf); static void NoteStoreCopied(void* data, StoreIOBuffer ioBuf); /// called by Store if the entry is no longer usable - static void Abort(void* param); + static void HandleStoreAbort(StoreToCommWriter *param); /// tell Comm to write action results void scheduleCommWrite(const StoreIOBuffer& ioBuf); diff --git a/src/store.cc b/src/store.cc index 2ad5cc1875..c69468f822 100644 --- a/src/store.cc +++ b/src/store.cc @@ -9,6 +9,7 @@ /* DEBUG: section 20 Storage Manager */ #include "squid.h" +#include "base/AsyncCbdataCalls.h" #include "base/TextException.h" #include "CacheDigest.h" #include "CacheManager.h" @@ -1124,19 +1125,9 @@ StoreEntry::abort() /* Notify the server side */ - /* - * DPW 2007-05-07 - * Should we check abort.data for validity? - */ - if (mem_obj->abort.callback) { - if (!cbdataReferenceValid(mem_obj->abort.data)) - debugs(20, DBG_IMPORTANT,HERE << "queueing event when abort.data is not valid"); - eventAdd("mem_obj->abort.callback", - mem_obj->abort.callback, - mem_obj->abort.data, - 0.0, - true); - unregisterAbort(); + if (mem_obj->abortCallback) { + ScheduleCallHere(mem_obj->abortCallback); + mem_obj->abortCallback = nullptr; } /* XXX Should we reverse these two, so that there is no @@ -1523,21 +1514,20 @@ StoreEntry::updateOnNotModified(const StoreEntry &e304) } void -StoreEntry::registerAbort(STABH * cb, void *data) +StoreEntry::registerAbortCallback(const AsyncCall::Pointer &handler) { assert(mem_obj); - assert(mem_obj->abort.callback == NULL); - mem_obj->abort.callback = cb; - mem_obj->abort.data = cbdataReference(data); + assert(!mem_obj->abortCallback); + mem_obj->abortCallback = handler; } void -StoreEntry::unregisterAbort() +StoreEntry::unregisterAbortCallback(const char *reason) { assert(mem_obj); - if (mem_obj->abort.callback) { - mem_obj->abort.callback = NULL; - cbdataReferenceDone(mem_obj->abort.data); + if (mem_obj->abortCallback) { + mem_obj->abortCallback->cancel(reason); + mem_obj->abortCallback = nullptr; } } diff --git a/src/tests/stub_libmgr.cc b/src/tests/stub_libmgr.cc index 7e19c71dd8..742da1ea54 100644 --- a/src/tests/stub_libmgr.cc +++ b/src/tests/stub_libmgr.cc @@ -233,7 +233,7 @@ bool Mgr::StoreToCommWriter::doneAll() const STUB_RETVAL(false) void Mgr::StoreToCommWriter::scheduleStoreCopy() STUB void Mgr::StoreToCommWriter::noteStoreCopied(StoreIOBuffer ioBuf) STUB void Mgr::StoreToCommWriter::NoteStoreCopied(void* data, StoreIOBuffer ioBuf) STUB -void Mgr::StoreToCommWriter::Abort(void* param) STUB +void Mgr::StoreToCommWriter::HandleStoreAbort(StoreToCommWriter *) STUB void Mgr::StoreToCommWriter::scheduleCommWrite(const StoreIOBuffer& ioBuf) STUB void Mgr::StoreToCommWriter::noteCommWrote(const CommIoCbParams& params) STUB void Mgr::StoreToCommWriter::noteCommClosed(const CommCloseCbParams& params) STUB diff --git a/src/tests/stub_store.cc b/src/tests/stub_store.cc index df895fe726..bcbebc55cb 100644 --- a/src/tests/stub_store.cc +++ b/src/tests/stub_store.cc @@ -57,11 +57,11 @@ void StoreEntry::ensureMemObject(const char *, const char *, const HttpRequestMe void StoreEntry::dump(int debug_lvl) const STUB void StoreEntry::hashDelete() STUB void StoreEntry::hashInsert(const cache_key *) STUB -void StoreEntry::registerAbort(STABH * cb, void *) STUB +void StoreEntry::registerAbortCallback(const AsyncCall::Pointer &) STUB void StoreEntry::reset() STUB void StoreEntry::setMemStatus(mem_status_t) STUB bool StoreEntry::timestampsSet() STUB_RETVAL(false) -void StoreEntry::unregisterAbort() STUB +void StoreEntry::unregisterAbortCallback(const char *) STUB void StoreEntry::destroyMemObject() STUB int StoreEntry::checkTooSmall() STUB_RETVAL(0) void StoreEntry::delayAwareRead(const Comm::ConnectionPointer&, char *buf, int len, AsyncCall::Pointer callback) STUB