bzp2010 commented on code in PR #13482:
URL: https://github.com/apache/apisix/pull/13482#discussion_r3417591974
##########
apisix/plugins/mcp/server_wrapper.lua:
##########
@@ -16,22 +16,39 @@
--
local ngx = ngx
local ngx_exit = ngx.exit
+local ngx_on_abort = ngx.on_abort
local re_match = ngx.re.match
+local pcall = pcall
local core = require("apisix.core")
local mcp_server = require("apisix.plugins.mcp.server")
+local session_limit = require("apisix.plugins.mcp.session_limit")
local _M = {}
local V241105_ENDPOINT_SSE = "sse"
local V241105_ENDPOINT_MESSAGE = "message"
+local DEFAULT_MAX_SESSIONS = 100
-local function sse_handler(conf, ctx, opts)
+
+-- run the SSE session loop; only returns once the client disconnects (or the
+-- connection setup fails). kept separate so that the caller can always release
+-- the resources it reserved, even if the loop raises.
+local function run_session(conf, opts, server)
-- send SSE headers and first chunk
core.response.set_header("Content-Type", "text/event-stream")
core.response.set_header("Cache-Control", "no-cache")
- local server = opts.server
+ -- stop the session as soon as the client goes away rather than waiting for
+ -- the next keepalive write to fail, so the backend process is released
+ -- promptly. requires lua_check_client_abort to be enabled; degrade to the
+ -- write-failure path if it is not.
+ local ok, err = ngx_on_abort(function()
+ server:stop()
+ end)
Review Comment:
This is a bit risky, as `ngx.on_abort` can only be called once per request.
If a plugin calls this hook, it effectively hijacks the call to that hook for
that single plugin. If other plugins or the core need to use this hook, it will
fail.
I suggest adding some comments to clarify this point.
##########
apisix/plugins/mcp/server_wrapper.lua:
##########
@@ -16,22 +16,39 @@
--
local ngx = ngx
local ngx_exit = ngx.exit
+local ngx_on_abort = ngx.on_abort
local re_match = ngx.re.match
+local pcall = pcall
local core = require("apisix.core")
local mcp_server = require("apisix.plugins.mcp.server")
+local session_limit = require("apisix.plugins.mcp.session_limit")
Review Comment:
This applies globally, right? That means no matter how many mcp-bridge
routes are configured, the total number of sessions cannot exceed the limit？
--
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]
