Aias00 commented on code in PR #7317:
URL: https://github.com/apache/shenyu/pull/7317#discussion_r4111192011
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/listener/websocket/WebsocketCollector.java:
##########
@@ -69,6 +70,12 @@ public class WebsocketCollector {
private static final Map<String, Set<Session>> NAMESPACE_SESSION_MAP =
Maps.newConcurrentMap();
private static final Map<Session, SessionSendQueue> SESSION_SEND_QUEUES =
Maps.newConcurrentMap();
+
+ /**
+ * Namespace captured at registration. {@code Session#isOpen()} is already
false when
+ * {@code @OnClose} runs, so the namespace cannot be read from the session
at teardown.
+ */
+ private static final Map<Session, String> SESSION_NAMESPACE_IDS =
Maps.newConcurrentMap();
Review Comment:
Non-blocking: this is a second source of truth keyed by `Session` that has
to stay in step with `NAMESPACE_SESSION_MAP`. Every teardown path goes through
`removeSessionIndexes`, so it is consistent today.
Two cheap hardening options:
1. State the invariant in this javadoc - "entry present iff the session is
in `NAMESPACE_SESSION_MAP`" - so a future teardown path cannot remove from one
and not the other.
2. Fall back to a full sweep when the remembered namespace is null:
```java
NAMESPACE_SESSION_MAP.values().forEach(sessions -> sessions.remove(session));
```
That is the approach #7318 takes for the whole cleanup. It costs
O(namespaces) only in the anomalous case, and it also cleans up a session that
somehow reached a namespace set without being registered here.
Also worth knowing for whoever merges: #7318 fixes the same issue with that
sweep and no extra map. I approved this one because it additionally stops
broadcast from traversing closed sessions and prunes empty sets - I checked
`NAMESPACE_SESSION_MAP` has no reader outside `WebsocketCollector` and its
test, so pruning is safe.
--
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]
