Sean-Walker0 opened a new pull request, #7318: URL: https://github.com/apache/shenyu/pull/7318
<!-- Describe your PR here; e.g. Fixes #issueNo --> Fixes #7315 `clearSession` is only invoked from `@OnClose`/`@OnError`, where the container has already closed the connection, so `session.isOpen()` is `false`. `getNamespaceId` returns `null` for closed sessions (it refuses to read `userProperties` unless the session is open), so the `NAMESPACE_SESSION_MAP` removal guarded by `StringUtils.isNotBlank(namespaceId)` can never run. The cleanup added in #5734 has therefore been **dead code since it landed** — the guard introduced by #5673 neuters it on the only two call paths. No test caught this because `WebsocketCollectorTest` kept `isOpen()` returning `true` while invoking `onClose`, the opposite of real container behavior. Consequences on master: every admin<->gateway websocket connection leaks one closed `Session` in the static namespace map per connect/disconnect cycle; each namespace broadcast later re-creates a `SessionSendQueue` for the stale session (`sendMessageBySession` uses `computeIfAbsent`), attempts `sendText` on the closed session, throws, and logs an error per broadcast per stale session. <!-- Thank you for proposing a pull request. This template will guide you through the essential steps necessary for a pull request. --> Make sure that: - [x] You have read the [contribution guidelines](https://shenyu.apache.org/community/contributor-guide). - [x] You submit test cases (unit or integration tests) that back your changes. - [x] Your local test passed `./mvnw test -pl shenyu-admin -am` and `./mvnw checkstyle:check -pl shenyu-admin` (module-scoped; full build left to CI). ### Modifications - `clearSession`: sweep the session out of every namespace set (`NAMESPACE_SESSION_MAP.values().forEach(s -> s.remove(session))`) instead of resolving the namespace via `getNamespaceId` — the sweep is idempotent, race-free and does not depend on reading a closed session. - `onOpen`: move `SESSION_SET.add(session)` after the namespace validation so a handshake that fails validation leaves no partially registered session (the secondary defect in #7315). ### Verifying this change - 4 new/extended tests in `WebsocketCollectorTest` model real container behavior (`isOpen() == false` when the close/error callback fires): - `testOnCloseRemovesClosedSessionFromNamespaceMap` — closed session removed from the namespace map, - `testOnErrorRemovesClosedSessionFromNamespaceMap` — same via `@OnError`, - `testRepeatedReconnectsDoNotGrowNamespaceSessionSet` — 3 reconnect cycles leave no residue (was: 3 leaked sessions), - `testOnOpenWithBlankNamespaceIdThrows` — now also asserts no partial registration in `SESSION_SET`/namespace map. - All four fail on current master (red run: `expected: <0> but was: <1>` / `<3>`) and pass with this change; full `shenyu-admin` module suite green; checkstyle green. ### Notes - Behavior change: closed sessions are now actually removed from `NAMESPACE_SESSION_MAP` (and invalid handshakes no longer land in `SESSION_SET`); namespace broadcasts iterate live sessions only instead of accumulating stale ones. - Intentionally out of scope from #7315: pruning empty namespace sets (races with concurrent `computeIfAbsent` + `add` in `onOpen` unless registration is restructured) and per-namespace session-count metrics (feature work, not a defect fix). - Orthogonal to the open websocket-related PRs: #7272/#7171 touch only `shenyu-client-spring-websocket`, #7094/#7095/#7035 touch `WebsocketDataChangedListener`/`WebsocketDataHandler`/`BaseDataCache` — none modify `WebsocketCollector`. -- 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]
