trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From zw...@apache.org
Subject [trafficserver] 04/07: Get appropriate locks on SSN_START hook delays (#7295)
Date Thu, 03 Dec 2020 19:36:27 GMT
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 <shinrich@yahoo-inc.com>
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);
 }


Mime
View raw message