Copilot commented on code in PR #13045:
URL: https://github.com/apache/trafficserver/pull/13045#discussion_r3018997968
##########
plugins/lua/ts_lua.cc:
##########
@@ -827,6 +827,46 @@ globalHookHandler(TSCont contp, TSEvent event ATS_UNUSED,
void *edata)
return 0;
}
+static int
+shutdownHookHandler(TSCont contp, TSEvent /* event ATS_UNUSED */, void * /*
edata ATS_UNUSED */)
+{
+ ts_lua_instance_conf *const conf = (ts_lua_instance_conf
*)TSContDataGet(contp);
+
+ for (int index = 0; index < conf->states; ++index) {
+ ts_lua_main_ctx *const main_ctx = &ts_lua_g_main_ctx_array[index];
+
+ TSMutexLock(main_ctx->mutexp);
+
+ lua_State *const L = main_ctx->lua;
+
+ // Restore the conf-specific global table so lua_getglobal resolves
+ // functions from the loaded script, matching ts_lua_reload_module.
+ lua_pushlightuserdata(L, conf);
+ lua_rawget(L, LUA_REGISTRYINDEX);
+ lua_replace(L, LUA_GLOBALSINDEX);
+
+ lua_getglobal(L, TS_LUA_FUNCTION_G_SHUT_DOWN);
+
+ if (lua_type(L, -1) == LUA_TFUNCTION) {
+ if (lua_pcall(L, 0, 0, 0) != 0) {
+ TSError("[ts_lua][%s] lua_pcall failed: %s", __FUNCTION__,
lua_tostring(L, -1));
Review Comment:
The shutdown hook error log doesn't include which Lua script/state failed.
Including conf->script and the state index (or similar identifier) would make
this actionable when multiple tslua instances/states are configured.
```suggestion
TSError("[ts_lua][%s] lua_pcall failed for script '%s' state %d:
%s", __FUNCTION__, conf->script, index,
lua_tostring(L, -1));
```
##########
plugins/lua/ts_lua.cc:
##########
@@ -1059,6 +1099,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__);
Review Comment:
If TSContCreate() fails here, the mutex created via TSMutexCreate() is
leaked because ownership is only transferred on successful continuation
creation. Create the mutex in a separate variable and call TSMutexDestroy() on
the failure path (or pass nullptr if a mutex is not needed).
```suggestion
TSMutex shutdown_mutex = TSMutexCreate();
TSCont shutdown_contp = TSContCreate(shutdownHookHandler,
shutdown_mutex);
if (!shutdown_contp) {
TSError("[ts_lua][%s] could not create shutdown continuation",
__FUNCTION__);
if (shutdown_mutex) {
TSMutexDestroy(shutdown_mutex);
}
```
--
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]
