This is an automated email from the ASF dual-hosted git repository. zwoop pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit 1264746ba577158217a452a4295a22e2610a34d6 Author: Sudheer Vinukonda <sudhe...@apache.org> AuthorDate: Wed May 20 09:13:58 2020 -0700 Add TXN_CLOSE hook to CPPAPI TransactionPlugin (#6800) * Add TXN_CLOSE hook to CPPAPI TransactionPlugin * Clean up TransactionPlugin object and associated Continuation in txn_close * Address review comments * More review comments (cherry picked from commit 34b57fccb40ef711ce2e6b31042c96efc74c0ecc) --- include/tscpp/api/Plugin.h | 12 ++++++++++- include/tscpp/api/TransactionPlugin.h | 4 ++++ src/tscpp/api/GlobalPlugin.cc | 1 + src/tscpp/api/utils_internal.cc | 39 ++++++++++++++++++++++++++++------- 4 files changed, 48 insertions(+), 8 deletions(-) diff --git a/include/tscpp/api/Plugin.h b/include/tscpp/api/Plugin.h index 2f57352..f37b805 100644 --- a/include/tscpp/api/Plugin.h +++ b/include/tscpp/api/Plugin.h @@ -58,7 +58,8 @@ public: HOOK_READ_REQUEST_HEADERS, /**< This hook will be fired after the request is read. */ HOOK_READ_CACHE_HEADERS, /**< This hook will be fired after the CACHE hdrs. */ HOOK_CACHE_LOOKUP_COMPLETE, /**< This hook will be fired after cache lookup complete. */ - HOOK_SELECT_ALT /**< This hook will be fired after select alt. */ + HOOK_TXN_CLOSE, /**< This hook will be fired after send response headers, only for TransactionPlugins::registerHook()!. */ + HOOK_SELECT_ALT /**< This hook will be fired after select alt. */ }; /** @@ -143,6 +144,15 @@ public: }; /** + * This method must be implemented when you hook HOOK_TXN_CLOSE + */ + virtual void + handleTxnClose(Transaction &transaction) + { + transaction.resume(); + }; + + /** * This method must be implemented when you hook HOOK_SELECT_ALT */ virtual void handleSelectAlt(const Request &clientReq, const Request &cachedReq, const Response &cachedResp){}; diff --git a/include/tscpp/api/TransactionPlugin.h b/include/tscpp/api/TransactionPlugin.h index b34fba0..ce3f1ca 100644 --- a/include/tscpp/api/TransactionPlugin.h +++ b/include/tscpp/api/TransactionPlugin.h @@ -93,6 +93,10 @@ public: * see HookType and Plugin for the correspond HookTypes and callback methods. If you fail to implement the * callback, a default implementation will be used that will only resume the Transaction. * + * \note For automatic destruction, you must either register dynamically allocated instances of + * classes derived from this class with the the corresponding Transaction object (using + * Transaction::addPlugin()), or register HOOK_TXN_CLOSE (but not both). + * * @param HookType the type of hook you wish to register * @see HookType * @see Plugin diff --git a/src/tscpp/api/GlobalPlugin.cc b/src/tscpp/api/GlobalPlugin.cc index b1be230..8e5f05c 100644 --- a/src/tscpp/api/GlobalPlugin.cc +++ b/src/tscpp/api/GlobalPlugin.cc @@ -87,6 +87,7 @@ GlobalPlugin::~GlobalPlugin() void GlobalPlugin::registerHook(Plugin::HookType hook_type) { + assert(hook_type != Plugin::HOOK_TXN_CLOSE); TSHttpHookID hook_id = utils::internal::convertInternalHookToTsHook(hook_type); TSHttpHookAdd(hook_id, state_->cont_); LOG_DEBUG("Registered global plugin %p for hook %s", this, HOOK_TYPE_STRINGS[hook_type].c_str()); diff --git a/src/tscpp/api/utils_internal.cc b/src/tscpp/api/utils_internal.cc index 7cb86e0..61f9044 100644 --- a/src/tscpp/api/utils_internal.cc +++ b/src/tscpp/api/utils_internal.cc @@ -49,6 +49,25 @@ resetTransactionHandles(Transaction &transaction, TSEvent event) return; } +void +cleanupTransaction(Transaction &transaction, TSHttpTxn ats_txn_handle) +{ + delete &transaction; + // reset the txn arg to prevent use-after-free + TSUserArgSet(ats_txn_handle, TRANSACTION_STORAGE_INDEX, nullptr); +} + +void +cleanupTransactionPlugin(Plugin *plugin) +{ + TransactionPlugin *transaction_plugin = static_cast<TransactionPlugin *>(plugin); + std::shared_ptr<Mutex> trans_mutex = utils::internal::getTransactionPluginMutex(*transaction_plugin); + LOG_DEBUG("Locking TransactionPlugin mutex to delete transaction plugin at %p", transaction_plugin); + trans_mutex->lock(); + delete transaction_plugin; + trans_mutex->unlock(); +} + int handleTransactionEvents(TSCont cont, TSEvent event, void *edata) { @@ -77,14 +96,9 @@ handleTransactionEvents(TSCont cont, TSEvent event, void *edata) resetTransactionHandles(transaction, event); const std::list<TransactionPlugin *> &plugins = utils::internal::getTransactionPlugins(transaction); for (auto plugin : plugins) { - std::shared_ptr<Mutex> trans_mutex = utils::internal::getTransactionPluginMutex(*plugin); - LOG_DEBUG("Locking TransactionPlugin mutex to delete transaction plugin at %p", plugin); - trans_mutex->lock(); - LOG_DEBUG("Locked Mutex...Deleting transaction plugin at %p", plugin); - delete plugin; - trans_mutex->unlock(); + cleanupTransactionPlugin(plugin); } - delete &transaction; + cleanupTransaction(transaction, ats_txn_handle); } break; default: assert(false); /* we should never get here */ @@ -141,6 +155,15 @@ void inline invokePluginForEvent(Plugin *plugin, TSHttpTxn ats_txn_handle, TSEve case TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE: plugin->handleReadCacheLookupComplete(transaction); break; + case TS_EVENT_HTTP_TXN_CLOSE: + if (plugin) { + plugin->handleTxnClose(transaction); + cleanupTransactionPlugin(plugin); + } else { + LOG_ERROR("stray event TS_EVENT_HTTP_TXN_CLOSE, no transaction plugin to handle it!"); + } + cleanupTransaction(transaction, ats_txn_handle); + break; default: assert(false); /* we should never get here */ break; @@ -191,6 +214,8 @@ utils::internal::convertInternalHookToTsHook(Plugin::HookType hooktype) return TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK; case Plugin::HOOK_SELECT_ALT: return TS_HTTP_SELECT_ALT_HOOK; + case Plugin::HOOK_TXN_CLOSE: + return TS_HTTP_TXN_CLOSE_HOOK; default: assert(false); // shouldn't happen, let's catch it early break;