You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by zw...@apache.org on 2020/05/20 16:45:41 UTC
[trafficserver] 06/06: Add TXN_CLOSE hook to CPPAPI
TransactionPlugin (#6800)
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 <su...@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;