You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2026/08/17 19:34:04 UTC
[PR] lua: run __shutdown__ with every Lua state quiesced [trafficserver]
bneradt opened a new pull request, #13556:
URL: https://github.com/apache/trafficserver/pull/13556
A __shutdown__ function usually releases process global resources, and
often does so through FFI into a native library. The shutdown handler
locked only the state it was calling the function on and released that
lock before moving to the next state, so the callback running in state 0
could tear those resources down while a request callback was still using
them in state 1. Production has crashed this way during restart, inside
a global read-request callback rather than in any shutdown path.
Locking every Lua state for the duration of the callbacks is what makes
__shutdown__ exclusive with all Lua code, but the lifecycle thread
cannot be the one holding those locks: ProxyMutex is recursive per event
thread, so that thread still enters Lua itself when it returns to its
event loop for a final iteration. This patch therefore hands the
callbacks to a thread of its own, which acquires every main state mutex
before invoking any __shutdown__ function and keeps the global ones for
the rest of the process lifetime, so no queued callback can enter Lua
after the resources it uses are gone. Acquisition is bounded, and the
callbacks are skipped with an error rather than run against a state that
never went idle. The remap state mutexes are given back once the
callbacks are done, because ATS destroys the remap instances after this
hook and that path takes those same mutexes.
The callbacks for every script are also invoked from a single
continuation now, so that the states are quiesced exactly once no matter
how many scripts define __shutdown__.
The gold test drives requests through a second Lua state across the
shutdown and fails if a __shutdown__ function overlaps one, or if any
Lua runs after the callbacks.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] lua: run __shutdown__ with every Lua state quiesced [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13556:
URL: https://github.com/apache/trafficserver/pull/13556#issuecomment-5736923503
Yes—the case this barrier protects is a native/FFI resource shared by global and remap scripts. If a global `__shutdown__` releases that resource while a remap callback still uses it, locking only the global states leaves the same use-after-free window. `__clean__` is appropriate for remap-instance-owned resources, but cannot protect a shared resource released by the global shutdown callback.
This coverage applies when `proxy.config.plugin.dynamic_reload_mode=0`, so the global and remap instances use the same plugin image. With dynamic reload enabled, the private remap copies have separate state arrays that this barrier cannot reach; the documentation already cautions against releasing resources shared across those copies from `__shutdown__`. I have clarified the shared-resource rationale and that queued callbacks remain blocked until process exit, even after the shutdown functions return.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] lua: run __shutdown__ with every Lua state quiesced [trafficserver]
Posted by "shukitchan (via GitHub)" <gi...@apache.org>.
shukitchan commented on PR #13556:
URL: https://github.com/apache/trafficserver/pull/13556#issuecomment-5502667040
why does ``__shutdown__`` need to take care of locking remap state as well?
this is tying global and remap state together unnecessarily?
if remap state is using some resources, it should be released through ``__clean__`` method
unless you are saying the some resources are shared between global and remap states?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] lua: run __shutdown__ with every Lua state quiesced [trafficserver]
Posted by "cmcfarlen (via GitHub) via github" <gi...@trafficserver.apache.org>.
cmcfarlen commented on PR #13556:
URL: https://github.com/apache/trafficserver/pull/13556#issuecomment-6532929710
Cherry-picked to the 10.2.x branch as 314a12edd7 for the 10.2.2 release.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] lua: run __shutdown__ with every Lua state quiesced [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13556:
URL: https://github.com/apache/trafficserver/pull/13556#discussion_r3909170164
##########
tests/gold_tests/pluginTest/lua/lua_global_shutdown.test.py:
##########
@@ -25,40 +28,172 @@
Test.ContinueOnFail = True
+# Helper script for signaling a traffic_server process by command-line identifier
+# match. Reused from gold_tests/logging.
+TS_PID_SCRIPT = 'ts_process_handler.py'
+
server = Test.MakeOriginServer("server")
-ts = Test.MakeATSProcess("ts")
+
+# The identifier shutdown_race_client.py matches on to find this process.
+ts = Test.MakeATSProcess("lua_shutdown_ts")
Review Comment:
The comment says the identifier is `shutdown_race_client.py`, but the helper script is passed `lua_shutdown_ts` (the ATS process name/identifier). This mismatch can confuse future maintenance/debugging of the test.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] lua: run __shutdown__ with every Lua state quiesced [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13556:
URL: https://github.com/apache/trafficserver/pull/13556#discussion_r3909544353
##########
doc/admin-guide/plugins/lua.en.rst:
##########
@@ -137,6 +137,27 @@ Example::
ts.debug('ATS shutting down, cleaning up resources')
end
+Because ``__shutdown__`` commonly releases process global resources, no Lua code
+runs in any of the plugin's Lua states while the ``__shutdown__`` functions are
+invoked: the plugin waits for every state to become idle first, and no Lua
+callback enters a state after that. If a state is still executing Lua after five
+seconds, the ``__shutdown__`` functions are skipped rather than run concurrently
+with it, and that is reported in the error log. Requests are held for as long as a
+``__shutdown__`` function runs, so it should return promptly.
Review Comment:
The implementation deliberately retains every state mutex after the shutdown callbacks return, so queued requests remain blocked until process exit—not only while a `__shutdown__` function runs. Please document that lifetime accurately so operators understand the terminal behavior.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] lua: run __shutdown__ with every Lua state quiesced [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #13556:
URL: https://github.com/apache/trafficserver/pull/13556
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] lua: run __shutdown__ with every Lua state quiesced [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13556:
URL: https://github.com/apache/trafficserver/pull/13556#discussion_r4051148374
##########
tests/gold_tests/pluginTest/lua/lua_global_shutdown.test.py:
##########
@@ -25,40 +28,172 @@
Test.ContinueOnFail = True
+# Helper script for signaling a traffic_server process by command-line identifier
+# match. Reused from gold_tests/logging.
+TS_PID_SCRIPT = 'ts_process_handler.py'
+
server = Test.MakeOriginServer("server")
-ts = Test.MakeATSProcess("ts")
+
+# The identifier shutdown_race_client.py matches on to find this process.
+ts = Test.MakeATSProcess("lua_shutdown_ts")
Review Comment:
Fixed in faa4712b95: the comment now explicitly identifies lua_shutdown_ts as the identifier matched by shutdown_race_client.py. Formatting passed, and the Python AST is unchanged.
##########
doc/admin-guide/plugins/lua.en.rst:
##########
@@ -137,6 +137,27 @@ Example::
ts.debug('ATS shutting down, cleaning up resources')
end
+Because ``__shutdown__`` commonly releases process global resources, no Lua code
+runs in any of the plugin's Lua states while the ``__shutdown__`` functions are
+invoked: the plugin waits for every state to become idle first, and no Lua
+callback enters a state after that. If a state is still executing Lua after five
+seconds, the ``__shutdown__`` functions are skipped rather than run concurrently
+with it, and that is reported in the error log. Requests are held for as long as a
+``__shutdown__`` function runs, so it should return promptly.
Review Comment:
Fixed in faa4712b95: the documentation now states that all state mutexes are retained until process exit and that requests waiting to enter those states remain blocked even after the shutdown functions return. The documentation build passed with Sphinx warnings treated as errors.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] lua: run __shutdown__ with every Lua state quiesced [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13556:
URL: https://github.com/apache/trafficserver/pull/13556#issuecomment-5319927271
I think there is still a remap-state re-entry window at `unlockStates(remap_states)`.
The lifecycle hook returns before `AutoStopCont` calls `shutdown_url_rewrite()` and `TSSystemState::shut_down_event_system()`. An existing `HttpSM` retains a `shared_ptr` lease on the remap configuration, so a remap callback already blocked on one of these mutexes can acquire it as soon as the barrier releases it and enter Lua after the global `__shutdown__` has freed shared process/FFI resources. That preserves the crash race when `proxy.config.plugin.dynamic_reload_mode=0`. The new AuTest drives shutdown load only through the global hook; the remap Lua script is exercised once before shutdown, so it does not detect this re-entry path.
My suggested fix is to add a per-state execution-stop flag, for example `ts_lua_main_ctx::is_shutting_down`, protected by that state's mutex:
1. After the barrier has acquired every global and remap state mutex, but before invoking any `__shutdown__`, set `is_shutting_down = true` on every state.
2. In every runtime Lua entry/resume path, check the flag immediately after acquiring the state mutex. If it is set, skip Lua and perform the appropriate ATS-side reenable/cleanup. This includes the global/remap/vconn handlers and coroutine, transform, intercept, and fetch resume paths.
3. Keep explicit teardown such as `ts_lua_del_module()` exempt from the runtime gate, so `TSRemapDeleteInstance` can still acquire the released remap mutexes and invoke `__clean__`.
4. The global locks can remain held as this patch already does; the remap locks can then be released safely because queued callbacks will acquire them, observe the flag, and decline to enter Lua.
Because the flag is set only while the barrier owns all state mutexes and is read only after acquiring the corresponding mutex, it does not need to be atomic: callbacks that were already executing drain before the barrier completes, while callbacks queued behind it see the stopped state.
I would also extend the AuTest with a remap Lua callback held/queued across SIGTERM and assert that it never logs execution after the global shutdown marker.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org