You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by wk...@apache.org on 2022/12/20 01:54:48 UTC
[trafficserver] branch master updated: Au test spawning a blocking thread in a transaction hook continuation. (#9044)
This is an automated email from the ASF dual-hosted git repository.
wkaras pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new ac0c7640c Au test spawning a blocking thread in a transaction hook continuation. (#9044)
ac0c7640c is described below
commit ac0c7640cc6fcc299f40a4fda3a3d207f95b50f0
Author: Walt Karas <wk...@yahoo-inc.com>
AuthorDate: Mon Dec 19 19:54:39 2022 -0600
Au test spawning a blocking thread in a transaction hook continuation. (#9044)
Getting a result from it, without blocking event task.
Co-authored-by: Walt Karas <wk...@yahooinc.com>
---
NOTICE | 2 +-
build/plugins.mk | 4 -
.../api/functions/TSHttpHookAdd.en.rst | 22 +-
.../api/functions/TSMutexLock.en.rst | 3 +
.../api/functions/TSMutexUnlock.en.rst | 6 +
.../continuations/writing-handler-functions.en.rst | 7 +
plugins/experimental/fastcgi/src/Readme | 1 -
tests/Makefile.am | 1 +
.../pluginTest/polite_hook_wait/Makefile.inc | 18 ++
.../pluginTest/polite_hook_wait/curl.gold | 2 +
.../polite_hook_wait/polite_hook_wait.cc | 272 +++++++++++++++++++++
.../polite_hook_wait/polite_hook_wait.test.py | 73 ++++++
12 files changed, 398 insertions(+), 13 deletions(-)
diff --git a/NOTICE b/NOTICE
index 4b9ffb79a..949f85201 100644
--- a/NOTICE
+++ b/NOTICE
@@ -35,7 +35,7 @@ Copyright (C) 2013 GoDaddy Operating Company, LLC
~~~
-lib/cppapi developed by LinkedIn
+include/tscpp/api, src/tscpp/api developed by LinkedIn
Copyright (c) 2013 LinkedIn
~~~
diff --git a/build/plugins.mk b/build/plugins.mk
index 4556fc22e..a0f0d3269 100644
--- a/build/plugins.mk
+++ b/build/plugins.mk
@@ -25,10 +25,6 @@ TS_PLUGIN_LD_FLAGS = \
-export-symbols-regex '^(TSRemapInit|TSRemapDone|TSRemapDoRemap|TSRemapNewInstance|TSRemapDeleteInstance|TSRemapOSResponse|TSPluginInit|TSRemapPreConfigReload|TSRemapPostConfigReload)$$'
TS_PLUGIN_CPPFLAGS = \
- -I$(abs_top_builddir)/proxy/api \
- -I$(abs_top_srcdir)/proxy/api \
- -I$(abs_top_srcdir)/include/cppapi/include \
- -I$(abs_top_builddir)/lib/cppapi/include \
-I$(abs_top_srcdir)/include \
-I$(abs_top_srcdir)/lib
diff --git a/doc/developer-guide/api/functions/TSHttpHookAdd.en.rst b/doc/developer-guide/api/functions/TSHttpHookAdd.en.rst
index 9536a3f88..b2c80b1ec 100644
--- a/doc/developer-guide/api/functions/TSHttpHookAdd.en.rst
+++ b/doc/developer-guide/api/functions/TSHttpHookAdd.en.rst
@@ -44,11 +44,13 @@ function for callback amounts to adding the function to a hook. You
can register your plugin to be called back for every single
transaction, or for specific transactions only.
-HTTP :term:`transaction` hooks are set on a global basis using the function
-:func:`TSHttpHookAdd`. This means that the continuation specified
-as the parameter to :func:`TSHttpHookAdd` is called for every
-transaction. :func:`TSHttpHookAdd` must only be called from
-:func:`TSPluginInit` or :func:`TSRemapInit`.
+HTTP :term:`transaction` and :term:`session` hooks are set on a
+global basis using the function :func:`TSHttpHookAdd`. This means
+that the continuation specified as the parameter to :func:`TSHttpHookAdd`
+is called for every transaction. :func:`TSHttpHookAdd` must only be called from
+:func:`TSPluginInit` or :func:`TSRemapInit`. Continuations set on a
+global hook will run before any continuations set on the session/transaction
+hook with the same hook ID.
:func:`TSHttpSsnHookAdd` adds :arg:`contp` to
the end of the list of HTTP :term:`session` hooks specified by :arg:`id`.
@@ -56,14 +58,20 @@ This means that :arg:`contp` is called back for every transaction
within the session, at the point specified by the hook ID. Since
:arg:`contp` is added to a session, it is not possible to call
:func:`TSHttpSsnHookAdd` from the plugin initialization routine;
-the plugin needs a handle to an HTTP session.
+the plugin needs a handle to an HTTP session. Continuations set on a
+session hook will run before any continuations set on the transaction
+hook with the same hook ID. This fucnction can be called from handler
+functions of continuations on a global per-session hook, including for
+the session hook with the same ID.
:func:`TSHttpTxnHookAdd` adds :arg:`contp`
to the end of the list of HTTP transaction hooks specified by
:arg:`id`. Since :arg:`contp` is added to a transaction, it is
not possible to call :func:`TSHttpTxnHookAdd` from the plugin
initialization routine but only when the plugin has a handle to an
-HTTP transaction.
+HTTP transaction. This fucnction can be called from handler
+functions of continuations on a global or session per-transaction
+hook, including the for transaction hook with the same ID.
A single continuation can be attached to multiple hooks at the same time.
It is good practice to conserve resources by reusing hooks in this way
diff --git a/doc/developer-guide/api/functions/TSMutexLock.en.rst b/doc/developer-guide/api/functions/TSMutexLock.en.rst
index 11558f2db..3cfc0bdbc 100644
--- a/doc/developer-guide/api/functions/TSMutexLock.en.rst
+++ b/doc/developer-guide/api/functions/TSMutexLock.en.rst
@@ -32,3 +32,6 @@ Synopsis
Description
===========
+
+Locks the mutex (recursively). Should only be called from a continuation
+handler function or a :type:`TSThread` function.
diff --git a/doc/developer-guide/api/functions/TSMutexUnlock.en.rst b/doc/developer-guide/api/functions/TSMutexUnlock.en.rst
index 37a96a99f..6f34bc85b 100644
--- a/doc/developer-guide/api/functions/TSMutexUnlock.en.rst
+++ b/doc/developer-guide/api/functions/TSMutexUnlock.en.rst
@@ -32,3 +32,9 @@ Synopsis
Description
===========
+
+Decrements the recursive lock count. If the count thus becomes zero, unlocks
+the mutex. This should only be called within the continuation handler function or
+:type:`TSThread` function that locked the mutex. Can also be called within
+the continuation handler for the continuation's mutex. (This is normally done
+before the continution handler destroys the continuation running it.)
diff --git a/doc/developer-guide/plugins/continuations/writing-handler-functions.en.rst b/doc/developer-guide/plugins/continuations/writing-handler-functions.en.rst
index f081ff707..b609cd35e 100644
--- a/doc/developer-guide/plugins/continuations/writing-handler-functions.en.rst
+++ b/doc/developer-guide/plugins/continuations/writing-handler-functions.en.rst
@@ -140,3 +140,10 @@ The continuation functions are listed below:
- :func:`TSContSchedule`
- :func:`TSContScheduleOnPool`
- :func:`TSContScheduleOnThread`
+
+When a handler function blocks, it blocks the event thread running it. This blocks all the continuations (internal ones
+along with those of plugins) in the event thread's queue. This may increase the worst-case latency for HTTP request
+processing. If there is enough blocking, this could increase CPU idle time, which may reduce proxy throughput. The
+Au test **polite_hook_wait** illustrates a method for using dynamic threading to do a blocking call without blocking
+any handler function. But the overhead of this method may cancel out the performance improvement, if blocking times
+are short.
diff --git a/plugins/experimental/fastcgi/src/Readme b/plugins/experimental/fastcgi/src/Readme
index 24cb8c608..20eb09352 100644
--- a/plugins/experimental/fastcgi/src/Readme
+++ b/plugins/experimental/fastcgi/src/Readme
@@ -32,7 +32,6 @@ tsxs -o ats_fastcgi.so \
-c ats_fastcgi.cc \
-L "${ats_dir}lib/" \
-I "${ats_dir}lib" \
- -I "${ats_dir}lib/cppapi/include/" \
-c ats_fcgi_client.cc \
-latscppapi
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 391078815..f3aa84a0a 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -33,6 +33,7 @@ AM_LDFLAGS += -rpath $(abs_builddir)
include gold_tests/bigobj/Makefile.inc
include gold_tests/continuations/plugins/Makefile.inc
include gold_tests/chunked_encoding/Makefile.inc
+include gold_tests/pluginTest/polite_hook_wait/Makefile.inc
include gold_tests/pluginTest/tsapi/Makefile.inc
include gold_tests/timeout/Makefile.inc
include gold_tests/tls/Makefile.inc
diff --git a/tests/gold_tests/pluginTest/polite_hook_wait/Makefile.inc b/tests/gold_tests/pluginTest/polite_hook_wait/Makefile.inc
new file mode 100644
index 000000000..ff87816ff
--- /dev/null
+++ b/tests/gold_tests/pluginTest/polite_hook_wait/Makefile.inc
@@ -0,0 +1,18 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+noinst_LTLIBRARIES += gold_tests/pluginTest/polite_hook_wait/polite_hook_wait.la
+gold_tests_pluginTest_polite_hook_wait_polite_hook_wait_la_SOURCES = gold_tests/pluginTest/polite_hook_wait/polite_hook_wait.cc
diff --git a/tests/gold_tests/pluginTest/polite_hook_wait/curl.gold b/tests/gold_tests/pluginTest/polite_hook_wait/curl.gold
new file mode 100644
index 000000000..be03905ce
--- /dev/null
+++ b/tests/gold_tests/pluginTest/polite_hook_wait/curl.gold
@@ -0,0 +1,2 @@
+> GET / HTTP/1.1
+< HTTP/1.1 403 Forbidden
diff --git a/tests/gold_tests/pluginTest/polite_hook_wait/polite_hook_wait.cc b/tests/gold_tests/pluginTest/polite_hook_wait/polite_hook_wait.cc
new file mode 100644
index 000000000..bba59407c
--- /dev/null
+++ b/tests/gold_tests/pluginTest/polite_hook_wait/polite_hook_wait.cc
@@ -0,0 +1,272 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <thread>
+#include <chrono>
+#include <atomic>
+
+#include <ts/ts.h>
+#include <tscpp/api/Cleanup.h>
+
+using atscppapi::TSContUniqPtr;
+using atscppapi::TSThreadUniqPtr;
+
+/*
+Test handling a blocking call (in a spawned thread) on a transaction hook without blocking the thread executing the hooks.
+
+It is dependent on continuations hooked globally on a transaction hook running before continations hooked for just the
+one transaction. It is dependent on the ability of a global continuation on a txn hook to add a per-txn continuation on
+the same hook.
+*/
+
+#define PINAME "polite_hook_wait"
+
+namespace
+{
+char PIName[] = PINAME;
+
+auto dbg_ctl{TSDbgCtlCreate(PIName)};
+
+enum Test_step
+{
+ BEGIN,
+ GLOBAL_CONT_READ_HDRS,
+ THREAD,
+ TXN_CONT_READ_HDRS,
+ END
+};
+
+char const *step_cstr(int test_step)
+{
+ char const *result{"BAD TEST STEP"};
+
+ switch (test_step)
+ {
+ case BEGIN:
+ result = "BEGIN";
+ break;
+
+ case GLOBAL_CONT_READ_HDRS:
+ result = "GLOBAL_CONT_READ_HDRS";
+ break;
+
+ case THREAD:
+ result = "THREAD";
+ break;
+
+ case TXN_CONT_READ_HDRS:
+ result = "TXN_CONT_READ_HDRS";
+ break;
+
+ default:
+ break;
+ }
+
+ return result;
+}
+
+int txn_count{0};
+
+void next_step(int curr)
+{
+ static std::atomic<int> test_step{BEGIN};
+
+ TSReleaseAssert(test_step.load(std::memory_order_relaxed) == curr);
+
+ if (BEGIN == curr) {
+ ++txn_count;
+
+ TSReleaseAssert(txn_count <= 2);
+ }
+
+ ++curr;
+ if (END == curr) {
+ curr = BEGIN;
+ }
+
+ TSDbg(dbg_ctl, "Entering test step %s", step_cstr(curr));
+
+ test_step.store(curr, std::memory_order_relaxed);
+}
+
+atscppapi::TxnAuxMgrData mgr_data;
+
+class Blocking_action
+{
+public:
+
+ static void init();
+
+private:
+
+ ~Blocking_action()
+ {
+ // This should either not block, or only block very briefly.
+ //
+ TSThreadWait(_checker.get());
+
+ TSDbg(dbg_ctl, "In ~Blocking_action()");
+ }
+
+ Blocking_action() = default;
+
+ static int _global_cont_func(TSCont, TSEvent event, void *eventData);
+ static int _txn_cont_func(TSCont, TSEvent event, void *eventData);
+ static void * _thread_func(void *vba);
+
+ TSContUniqPtr _txn_hook_cont{TSContCreate(_txn_cont_func, TSMutexCreate())};
+ std::atomic<bool> _cont_mutex_locked{false};
+
+ TSThreadUniqPtr _checker{TSThreadCreate(&_thread_func, this)};
+
+ bool txn_valid{false};
+
+ friend class atscppapi::TxnAuxDataMgr<Blocking_action, mgr_data>;
+};
+
+using AuxDataMgr = atscppapi::TxnAuxDataMgr<Blocking_action, mgr_data>;
+
+void
+Blocking_action::init()
+{
+ static TSContUniqPtr global{TSContCreate(_global_cont_func, nullptr)};
+
+ TSHttpHookAdd(TS_HTTP_READ_REQUEST_HDR_HOOK, global.get());
+ TSHttpHookAdd(TS_HTTP_SEND_RESPONSE_HDR_HOOK, global.get());
+}
+
+int Blocking_action::_global_cont_func(TSCont, TSEvent event, void *eventData)
+{
+ TSDbg(dbg_ctl, "entering _global_cont_func()");
+
+ TSReleaseAssert(eventData != nullptr);
+
+ TSHttpTxn txn{static_cast<TSHttpTxn>(eventData)};
+
+ switch (event)
+ {
+ case TS_EVENT_HTTP_READ_REQUEST_HDR: {
+ next_step(BEGIN);
+
+ Blocking_action &ba = AuxDataMgr::data(txn);
+
+ if (!ba._checker.get()) {
+ TSError(PINAME ": failed to create thread");
+ TSHttpTxnReenable(txn, TS_EVENT_HTTP_CONTINUE);
+ return 0;
+ }
+
+ TSHttpTxnHookAdd(txn, TS_HTTP_READ_REQUEST_HDR_HOOK, ba._txn_hook_cont.get());
+
+ while (!ba._cont_mutex_locked.load(std::memory_order_acquire)) {
+ std::this_thread::yield();
+ }
+ }
+ break;
+
+ case TS_EVENT_HTTP_SEND_RESPONSE_HDR:
+ next_step(TXN_CONT_READ_HDRS);
+
+ if (!AuxDataMgr::data(txn).txn_valid) {
+ static const char msg[] = "authorization denied\n";
+
+ TSHttpTxnErrorBodySet(txn, TSstrdup(msg), sizeof(msg) - 1, TSstrdup("text/plain"));
+ }
+
+ break;
+
+ default:
+ TSReleaseAssert(false);
+ break;
+ }
+
+ TSHttpTxnReenable(txn, TS_EVENT_HTTP_CONTINUE);
+
+ return 0;
+}
+
+void * Blocking_action::_thread_func(void *vba)
+{
+ next_step(GLOBAL_CONT_READ_HDRS);
+
+ auto ba = static_cast<Blocking_action *>(vba);
+
+ TSMutexLock(TSContMutexGet(ba->_txn_hook_cont.get())); // This will never block.
+ ba->_cont_mutex_locked.store(true, std::memory_order_release);
+
+ // This is a stand-in for some blocking call to validate the HTTP request in some way.
+ //
+ std::this_thread::sleep_for(std::chrono::milliseconds(200));
+
+ // Pass "validation" for first transaction, fail it for second.
+ //
+ if (1 == txn_count) {
+ ba->txn_valid = true;
+ }
+
+ // Let per-txn continuation run.
+ //
+ TSMutexUnlock(TSContMutexGet(ba->_txn_hook_cont.get()));
+
+ return nullptr;
+}
+
+int Blocking_action::_txn_cont_func(TSCont, TSEvent event, void *eventData)
+{
+ next_step(THREAD);
+
+ TSReleaseAssert(eventData != nullptr);
+ TSReleaseAssert(TS_EVENT_HTTP_READ_REQUEST_HDR == event);
+
+ TSHttpTxn txn{static_cast<TSHttpTxn>(eventData)};
+
+ Blocking_action &ba = AuxDataMgr::data(txn);
+
+ if (!ba.txn_valid) {
+ TSHttpTxnStatusSet(txn, TS_HTTP_STATUS_FORBIDDEN);
+ }
+
+ TSHttpTxnReenable(txn, ba.txn_valid ? TS_EVENT_HTTP_CONTINUE : TS_EVENT_HTTP_ERROR);
+
+ return 0;
+}
+
+} // end anonymous namespace
+
+void
+TSPluginInit(int n_arg, char const *arg[])
+{
+ TSDbg(dbg_ctl, "initializing plugin");
+
+ TSPluginRegistrationInfo info;
+
+ info.plugin_name = const_cast<char*>(PIName);
+ info.vendor_name = const_cast<char*>("apache");
+ info.support_email = const_cast<char*>("edge@yahooinc.com");
+
+ if (TSPluginRegister(&info) != TS_SUCCESS) {
+ TSError(PINAME ": failure calling TSPluginRegister.");
+ return;
+ } else {
+ TSDbg(dbg_ctl, "Plugin registration succeeded.");
+ }
+
+ AuxDataMgr::init(PIName);
+
+ Blocking_action::init();
+}
diff --git a/tests/gold_tests/pluginTest/polite_hook_wait/polite_hook_wait.test.py b/tests/gold_tests/pluginTest/polite_hook_wait/polite_hook_wait.test.py
new file mode 100644
index 000000000..8e865e763
--- /dev/null
+++ b/tests/gold_tests/pluginTest/polite_hook_wait/polite_hook_wait.test.py
@@ -0,0 +1,73 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+
+
+Test.Summary = '''
+Test spawning a thread in a transaction hook continuation, and getting a result from it, without blocking event task.
+'''
+
+plugin_name = "polite_hook_wait"
+
+server = Test.MakeOriginServer("server")
+
+request_header = {
+ "headers": "GET / HTTP/1.1\r\nHost: doesnotmatter\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
+response_header = {"headers": "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n", "timestamp": "1469733493.993", "body": "112233"}
+server.addResponse("sessionlog.json", request_header, response_header)
+
+
+# Disable the cache to make sure each request is forwarded to the origin
+# server.
+ts = Test.MakeATSProcess("ts", enable_cache=False, block_for_debug=False)
+
+ts.Disk.records_config.update({
+ 'proxy.config.proxy_name': 'Poxy_Proxy', # This will be the server name.
+ 'proxy.config.url_remap.remap_required': 1,
+ 'proxy.config.diags.debug.enabled': 3,
+ 'proxy.config.diags.debug.tags': f'{plugin_name}',
+})
+
+rp = os.path.join(Test.Variables.AtsBuildGoldTestsDir, 'pluginTest', 'polite_hook_wait', '.libs', f'{plugin_name}.so')
+ts.Setup.Copy(rp, ts.Env['PROXY_CONFIG_PLUGIN_PLUGIN_DIR'])
+
+ts.Disk.plugin_config.AddLine(
+ f"{plugin_name}.so"
+)
+
+ts.Disk.remap_config.AddLine(
+ "map http://myhost.test http://127.0.0.1:{0}".format(server.Variables.Port)
+)
+
+tr = Test.AddTestRun()
+tr.Processes.Default.StartBefore(server)
+tr.Processes.Default.StartBefore(ts)
+tr.Processes.Default.Command = (
+ 'curl --verbose --ipv4 --header "Host:myhost.test" http://localhost:{}/ 2>curl.txt'.format(ts.Variables.port)
+)
+tr.Processes.Default.ReturnCode = 0
+
+tr = Test.AddTestRun()
+tr.Processes.Default.Command = (
+ 'curl --verbose --ipv4 --header "Host:myhost.test" http://localhost:{}/ 2>curl.txt'.format(ts.Variables.port)
+)
+tr.Processes.Default.ReturnCode = 0
+
+tr = Test.AddTestRun()
+tr.Processes.Default.Command = "grep -F HTTP/ curl.txt"
+tr.Processes.Default.Streams.stdout = "curl.gold"
+tr.Processes.Default.ReturnCode = 0