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 c0ae43ccf9f67098795c024c20be5ee66662a26c Author: Susan Hinrichs <[email protected]> AuthorDate: Mon Nov 2 12:46:25 2020 -0600 Get appropriate locks on SSN_START hook delays (#7295) (cherry picked from commit 1765c9f2d6ade367773342983db6973015d70f42) --- proxy/http/HttpSessionAccept.cc | 2 + proxy/http2/Http2SessionAccept.cc | 3 ++ src/traffic_server/InkAPI.cc | 36 ++++++++++--- .../pluginTest/test_hooks/ssn_delay.gold | 8 +++ .../test_hooks/ssn_start_delay_hook.test.py | 60 ++++++++++++++++++++++ tests/tools/plugins/hook_add_plugin.cc | 35 +++++++++---- 6 files changed, 127 insertions(+), 17 deletions(-) diff --git a/proxy/http/HttpSessionAccept.cc b/proxy/http/HttpSessionAccept.cc index e7fd20f..ea9d075 100644 --- a/proxy/http/HttpSessionAccept.cc +++ b/proxy/http/HttpSessionAccept.cc @@ -54,6 +54,8 @@ HttpSessionAccept::accept(NetVConnection *netvc, MIOBuffer *iobuf, IOBufferReade new_session->accept_options = static_cast<Options *>(this); new_session->acl = std::move(acl); + // Pin session to current ET_NET thread + new_session->setThreadAffinity(this_ethread()); new_session->new_connection(netvc, iobuf, reader); new_session->trans.upstream_outbound_options = *new_session->accept_options; diff --git a/proxy/http2/Http2SessionAccept.cc b/proxy/http2/Http2SessionAccept.cc index 7f68e64..f0226fd 100644 --- a/proxy/http2/Http2SessionAccept.cc +++ b/proxy/http2/Http2SessionAccept.cc @@ -56,6 +56,9 @@ Http2SessionAccept::accept(NetVConnection *netvc, MIOBuffer *iobuf, IOBufferRead Http2ClientSession *new_session = THREAD_ALLOC_INIT(http2ClientSessionAllocator, this_ethread()); new_session->acl = std::move(session_acl); new_session->accept_options = &options; + + // Pin session to current ET_NET thread + new_session->setThreadAffinity(this_ethread()); new_session->new_connection(netvc, iobuf, reader); return true; diff --git a/src/traffic_server/InkAPI.cc b/src/traffic_server/InkAPI.cc index e02106b..4b2d9af 100644 --- a/src/traffic_server/InkAPI.cc +++ b/src/traffic_server/InkAPI.cc @@ -4895,7 +4895,7 @@ TSHttpSsnServerVConnGet(TSHttpSsn ssnp) class TSHttpSsnCallback : public Continuation { public: - TSHttpSsnCallback(ProxySession *cs, TSEvent event) : Continuation(cs->mutex), m_cs(cs), m_event(event) + TSHttpSsnCallback(ProxySession *cs, Ptr<ProxyMutex> m, TSEvent event) : Continuation(m), m_cs(cs), m_event(event) { SET_HANDLER(&TSHttpSsnCallback::event_handler); } @@ -4903,8 +4903,18 @@ public: int event_handler(int, void *) { - m_cs->handleEvent((int)m_event, nullptr); - delete this; + // The current continuation is associated with the nethandler mutex. + // We need to hold the nethandler mutex because the later Session logic may + // activate the nethandler add_to_queue logic + // Need to make sure we have the ProxySession mutex as well. + EThread *eth = this_ethread(); + MUTEX_TRY_LOCK(trylock, m_cs->mutex, eth); + if (!trylock.is_locked()) { + eth->schedule_imm(this); + } else { + m_cs->handleEvent((int)m_event, nullptr); + delete this; + } return 0; } @@ -4923,13 +4933,25 @@ TSHttpSsnReenable(TSHttpSsn ssnp, TSEvent event) // If this function is being executed on a thread created by the API // which is DEDICATED, the continuation needs to be called back on a - // REGULAR thread. - if (eth->tt != REGULAR) { - eventProcessor.schedule_imm(new TSHttpSsnCallback(cs, event), ET_NET); + // REGULAR thread. Specially an ET_NET thread + if (!eth->is_event_type(ET_NET)) { + EThread *affinity_thread = cs->getThreadAffinity(); + if (affinity_thread && affinity_thread->is_event_type(ET_NET)) { + NetHandler *nh = get_NetHandler(affinity_thread); + affinity_thread->schedule_imm(new TSHttpSsnCallback(cs, nh->mutex, event), ET_NET); + } else { + eventProcessor.schedule_imm(new TSHttpSsnCallback(cs, cs->mutex, event), ET_NET); + } } else { MUTEX_TRY_LOCK(trylock, cs->mutex, eth); if (!trylock.is_locked()) { - eventProcessor.schedule_imm(new TSHttpSsnCallback(cs, event), ET_NET); + EThread *affinity_thread = cs->getThreadAffinity(); + if (affinity_thread && affinity_thread->is_event_type(ET_NET)) { + NetHandler *nh = get_NetHandler(affinity_thread); + affinity_thread->schedule_imm(new TSHttpSsnCallback(cs, nh->mutex, event), ET_NET); + } else { + eventProcessor.schedule_imm(new TSHttpSsnCallback(cs, cs->mutex, event), ET_NET); + } } else { cs->handleEvent((int)event, nullptr); } diff --git a/tests/gold_tests/pluginTest/test_hooks/ssn_delay.gold b/tests/gold_tests/pluginTest/test_hooks/ssn_delay.gold new file mode 100644 index 0000000..0510f3f --- /dev/null +++ b/tests/gold_tests/pluginTest/test_hooks/ssn_delay.gold @@ -0,0 +1,8 @@ +`` DIAG: (test) -- globalHandler :: TS_EVENT_HTTP_SSN_START +`` DIAG: (test) New session, cont is `` +`` DIAG: (test) -- sessionHandler :: TS_EVENT_TIMEOUT +`` DIAG: (test) -- sessionHandler :: TS_EVENT_HTTP_PRE_REMAP +`` DIAG: (test) -- transactionHandler :: TS_EVENT_HTTP_PRE_REMAP +`` DIAG: (test) -- transactionHandler :: TS_EVENT_HTTP_TXN_CLOSE +`` DIAG: (test) -- sessionHandler :: TS_EVENT_HTTP_SSN_CLOSE +`` diff --git a/tests/gold_tests/pluginTest/test_hooks/ssn_start_delay_hook.test.py b/tests/gold_tests/pluginTest/test_hooks/ssn_start_delay_hook.test.py new file mode 100644 index 0000000..738630f --- /dev/null +++ b/tests/gold_tests/pluginTest/test_hooks/ssn_start_delay_hook.test.py @@ -0,0 +1,60 @@ +# 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 adding hooks, and rescheduling the ssn start hook from a non-net thread +''' + +Test.ContinueOnFail = True + +server = Test.MakeOriginServer("server") + +request_header = { + "headers": "GET /argh 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": ""} +server.addResponse("sessionlog.json", request_header, response_header) + +ts = Test.MakeATSProcess("ts", select_ports=True, enable_tls=False) + +ts.Disk.records_config.update({ + 'proxy.config.diags.debug.tags': 'test', + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.http.cache.http': 0, + 'proxy.config.url_remap.remap_required': 0, +}) + +Test.PrepareTestPlugin(os.path.join(Test.Variables.AtsTestPluginsDir, 'hook_add_plugin.so'), ts, '-delay') + +ts.Disk.remap_config.AddLine( + "map http://one http://127.0.0.1:{0}".format(server.Variables.Port) +) + +tr = Test.AddTestRun() +# Probe server port to check if ready. +tr.Processes.Default.StartBefore(server, ready=When.PortOpen(server.Variables.Port)) +# Probe TS cleartext port to check if ready (probing TLS port causes spurious VCONN hook triggers). +tr.Processes.Default.StartBefore(Test.Processes.ts, ready=When.PortOpen(ts.Variables.port)) +# +tr.Processes.Default.Command = ( + 'curl --verbose --ipv4 --header "Host: one" http://localhost:{0}/argh'.format(ts.Variables.port) +) +tr.Processes.Default.ReturnCode = 0 + +# Look at the debug output from the plugin +ts.Streams.All = "ssn_delay.gold" diff --git a/tests/tools/plugins/hook_add_plugin.cc b/tests/tools/plugins/hook_add_plugin.cc index f8fcfe4..87bc859 100644 --- a/tests/tools/plugins/hook_add_plugin.cc +++ b/tests/tools/plugins/hook_add_plugin.cc @@ -21,9 +21,13 @@ limitations under the License. */ #include <ts/ts.h> +#include <string.h> #define PLUGIN_TAG "test" +// Number of seconds to reschedule to a task thread and delay +int DelayStart = 0; + int transactionHandler(TSCont continuation, TSEvent event, void *d) { @@ -79,6 +83,12 @@ sessionHandler(TSCont continuation, TSEvent event, void *d) return 0; } break; + case TS_EVENT_TIMEOUT: { // The schedule case, reenable the session continuation + TSDebug(PLUGIN_TAG, " -- sessionHandler :: TS_EVENT_TIMEOUT"); + TSHttpSsn session = static_cast<TSHttpSsn>(TSContDataGet(continuation)); + TSHttpSsnReenable(session, TS_EVENT_HTTP_CONTINUE); + return 0; + } default: TSAssert(!"Unexpected event"); break; @@ -91,10 +101,8 @@ sessionHandler(TSCont continuation, TSEvent event, void *d) int globalHandler(TSCont continuation, TSEvent event, void *data) { - TSHttpSsn session = static_cast<TSHttpSsn>(data); - - switch (event) { - case TS_EVENT_HTTP_SSN_START: { + if (event == TS_EVENT_HTTP_SSN_START) { + TSHttpSsn session = static_cast<TSHttpSsn>(data); TSDebug(PLUGIN_TAG, " -- globalHandler :: TS_EVENT_HTTP_SSN_START"); TSCont cont = TSContCreate(sessionHandler, TSMutexCreate()); @@ -102,14 +110,15 @@ globalHandler(TSCont continuation, TSEvent event, void *data) TSHttpSsnHookAdd(session, TS_HTTP_SSN_CLOSE_HOOK, cont); TSDebug(PLUGIN_TAG, "New session, cont is %p", cont); - } break; - default: - return 0; + if (DelayStart == 0) { + TSHttpSsnReenable(session, TS_EVENT_HTTP_CONTINUE); + } else { + TSContDataSet(cont, session); + TSContScheduleOnPool(cont, 500, TS_THREAD_POOL_TASK); + } } - TSHttpSsnReenable(session, TS_EVENT_HTTP_CONTINUE); - return 0; } @@ -134,7 +143,13 @@ TSPluginInit(int argc, const char **argv) return; } - TSCont continuation = TSContCreate(globalHandler, nullptr); + if (argc >= 2) { + TSDebug(PLUGIN_TAG, "Argument %s", argv[1]); + if (strcmp(argv[1], "-delay") == 0) { + DelayStart = 1; + } + } + TSCont continuation = TSContCreate(globalHandler, TSMutexCreate()); TSHttpHookAdd(TS_HTTP_SSN_START_HOOK, continuation); }
