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 6f32877b02f1ad9d50b83412ca64c3acb7abc941..c468541b505683c3a209979e508fc6afca972053 100644 (file)
--- 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 f9cd6d37b26dcfce84b3f94592009ea93e3a5d2a..0e052f0c330b4ebae23e75dad89b6e4dd2fbf471 100644 (file)
--- 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 76686feb0bb603d3d33051a28fa25f8cb87c6bf4..c34b32007f83d76cbdfd6ad110f506246b75fa7e 100644 (file)
--- 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 33209db9033ffb4e0c2bd749e14355c234383490..7bf30d479bdd3d0e7404f6e869e3dc4a5b8b31a5 100644 (file)
--- 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 a141da5680c2c9fe98e4b5c436d40d9b7cc13b0d..19967b3e6280c7f25be9f7700fb37ce8f05a35a5 100644 (file)
--- 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 ed1206e29d1dc6081e713840870509803f058e34..251440eb92d0be0f173580e3c73ef90dd135a82d 100644 (file)
--- 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<StoreToCommWriter*>(param);
     if (Comm::IsConnOpen(mgrWriter->clientConnection))
         mgrWriter->clientConnection->close();
 }

diff --git a/src/mgr/StoreToCommWriter.h b/src/mgr/StoreToCommWriter.h
index 367d516549cc1f1cfb1ff7b3f6109f2e5de3b493..84fb1cfe8dffb5d021eb8ea93f397172506a0c60 100644 (file)
--- 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 2ad5cc187529ed2df98aeee5ef4267ce2ac10edd..c69468f822208a93149231b3c62c00dc2569b7e5 100644 (file)
--- 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 7e19c71dd87b7670ee036a40f1d5dcc9a78ab16d..742da1ea54993b58f1a863930043ff5463d8d866 100644 (file)
--- 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 df895fe7261acb3c4c6d4781b507092933ce5eb0..bcbebc55cb39edd8bcd1fb820cfcf9fb6fde8f0c 100644 (file)
--- 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