Copilot commented on code in PR #13045:
URL: https://github.com/apache/trafficserver/pull/13045#discussion_r3017257365
##########
plugins/lua/ts_lua_common.h:
##########
@@ -62,6 +62,9 @@ extern "C" {
// TLS hooks can only be global
#define TS_LUA_FUNCTION_G_VCONN_START "do_global_vconn_start"
+// Lifecycle hooks
+#define TS_LUA_FUNCTION_G_SHUT_DOWN "do_global_shut_down"
Review Comment:
The public Lua hook name `do_global_shut_down` is a bit inconsistent with
the common spelling “shutdown” (single word) used elsewhere (e.g.,
`TS_LIFECYCLE_SHUTDOWN_HOOK`). Since this is a newly introduced API, consider
renaming it to `do_global_shutdown` (and updating the macro, docs, and the new
tests/scripts) to improve discoverability and avoid locking in an awkward name.
```suggestion
#define TS_LUA_FUNCTION_G_SHUT_DOWN "do_global_shutdown"
```
##########
tests/gold_tests/pluginTest/lua/lua_global_shutdown.test.py:
##########
@@ -0,0 +1,65 @@
+'''
+Test do_global_shut_down lua global plugin hook.
+'''
+# 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.
+
+Test.Summary = '''
+Test do_global_shut_down lua global plugin hook
+'''
+
+Test.SkipUnless(Condition.PluginExists('tslua.so'),)
+
+Test.ContinueOnFail = True
+
+server = Test.MakeOriginServer("server")
+ts = Test.MakeATSProcess("ts")
+
+Test.Setup.Copy("global_shutdown.lua")
+
+request_header = {"headers": "GET / HTTP/1.1\r\nHost:
www.example.com\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("sessionfile.log", request_header, response_header)
+
+ts.Disk.remap_config.AddLine('map /
http://127.0.0.1:{}/'.format(server.Variables.Port))
+
+# Use 2 states so the shutdown handler is called a predictable number of times.
+ts.Disk.plugin_config.AddLine('tslua.so --states=2
{}/global_shutdown.lua'.format(Test.RunDirectory))
+
Review Comment:
The test configures `--states=2` and the comments state the hook should run
once per state, but the assertion only checks that the message appears at least
once. This can miss regressions where the shutdown hook runs for only one
state. Consider either (a) asserting two occurrences (or “at least 2”) of the
shutdown log line, or (b) switching to `--states=1` and updating the comments
to match what’s being asserted.
##########
plugins/lua/ts_lua.cc:
##########
@@ -1059,6 +1087,26 @@ TSPluginInit(int argc, const char *argv[])
ts_lua_destroy_vconn_ctx(vconn_ctx);
+ // adding shutdown hook if the lua global shutdown function exists
+ ts_lua_main_ctx *shutdown_main_ctx = &ts_lua_g_main_ctx_array[0];
+ ts_lua_http_ctx *shutdown_http_ctx =
ts_lua_create_http_ctx(shutdown_main_ctx, conf);
+ lua_State *sl = shutdown_http_ctx->cinfo.routine.lua;
+
+ lua_getglobal(sl, TS_LUA_FUNCTION_G_SHUT_DOWN);
+ if (lua_type(sl, -1) == LUA_TFUNCTION) {
+ TSCont shutdown_contp = TSContCreate(shutdownHookHandler, TSMutexCreate());
+ if (!shutdown_contp) {
+ TSError("[ts_lua][%s] could not create shutdown continuation",
__FUNCTION__);
+ } else {
+ TSContDataSet(shutdown_contp, conf);
+ TSLifecycleHookAdd(TS_LIFECYCLE_SHUTDOWN_HOOK, shutdown_contp);
+ Dbg(dbg_ctl, "shutdown_hook added");
+ }
+ }
+ lua_pop(sl, 1);
+
+ ts_lua_destroy_http_ctx(shutdown_http_ctx);
Review Comment:
`ts_lua_create_http_ctx()`/`ts_lua_destroy_http_ctx()` appear to be used
only to obtain a `lua_State` for a one-time `lua_getglobal` check. This adds
non-trivial setup/teardown work during plugin init and makes the logic more
complex than necessary. Consider checking `TS_LUA_FUNCTION_G_SHUT_DOWN`
directly on `shutdown_main_ctx->lua` (which is also what the shutdown handler
uses) to avoid allocating an HTTP context solely for feature detection.
##########
tests/gold_tests/pluginTest/lua/lua_global_shutdown.test.py:
##########
@@ -0,0 +1,65 @@
+'''
+Test do_global_shut_down lua global plugin hook.
+'''
+# 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.
+
+Test.Summary = '''
+Test do_global_shut_down lua global plugin hook
+'''
+
+Test.SkipUnless(Condition.PluginExists('tslua.so'),)
+
+Test.ContinueOnFail = True
+
+server = Test.MakeOriginServer("server")
+ts = Test.MakeATSProcess("ts")
+
+Test.Setup.Copy("global_shutdown.lua")
+
+request_header = {"headers": "GET / HTTP/1.1\r\nHost:
www.example.com\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("sessionfile.log", request_header, response_header)
+
+ts.Disk.remap_config.AddLine('map /
http://127.0.0.1:{}/'.format(server.Variables.Port))
+
+# Use 2 states so the shutdown handler is called a predictable number of times.
+ts.Disk.plugin_config.AddLine('tslua.so --states=2
{}/global_shutdown.lua'.format(Test.RunDirectory))
+
+ts.Disk.records_config.update({
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'ts_lua',
+})
+
+curl_and_args = '-s -D /dev/stdout -o /dev/stderr -x localhost:{}
'.format(ts.Variables.port)
+
+# 0 Test - Send a request to confirm the global plugin is active.
+tr = Test.AddTestRun("Lua global read request hook fires for HTTP requests")
+ps = tr.Processes.Default
+ps.StartBefore(server, ready=When.PortOpen(server.Variables.Port))
+ps.StartBefore(Test.Processes.ts)
+tr.MakeCurlCommand(curl_and_args + 'http://www.example.com/', ts=ts)
+ps.ReturnCode = 0
+tr.StillRunningAfter = ts
+
+# Verify do_global_read_request was invoked for the HTTP request above.
+ts.Disk.traffic_out.Content = Testers.ContainsExpression(
+ r'do_global_read_request called', 'do_global_read_request should be called
for HTTP requests')
+
+# After all test runs complete AuTest stops ATS, which fires
TS_LIFECYCLE_SHUTDOWN_HOOK.
+# The shutdown handler calls do_global_shut_down once per Lua state (2 states
configured).
+ts.Disk.traffic_out.Content += Testers.ContainsExpression(
+ r'do_global_shut_down called', 'do_global_shut_down should be called on
ATS shutdown')
Review Comment:
The test configures `--states=2` and the comments state the hook should run
once per state, but the assertion only checks that the message appears at least
once. This can miss regressions where the shutdown hook runs for only one
state. Consider either (a) asserting two occurrences (or “at least 2”) of the
shutdown log line, or (b) switching to `--states=1` and updating the comments
to match what’s being asserted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
