Aias00 commented on PR #6894: URL: https://github.com/apache/shenyu/pull/6894#issuecomment-5193352259
Good fix — the hardcoded `sessionPresent(true)` was a clear spec violation (MQTT-3.2.2-6/7), and the new derivation `!cleanSession && session != null` is correct, including the ordering (sessionPresent computed before the cleanSession overwrite). Two things worth addressing: **Subscription resume loses QoS.** `Connect.java` (~L91) calls `SubscribeRepository.add(new ArrayList<>(session.getTopics()), Collections.singletonList(ctx.channel()))`, which goes through the `add(List<String>, List<Channel>)` overload (`SubscribeRepository.java:43`) — a topic→channel mapping only. It bypasses the QoS-aware `add(Channel, List<MqttTopicSubscription>)` overload (L56). `MqttSession.topics` is a `Set<String>`, so the original QoS levels (and duplicate subscriptions with differing QoS) are collapsed; resumed subscriptions get an implicit default QoS rather than the client's original. Could `MqttSession` store `List<MqttTopicSubscription>` (or `Map<String, MqttQoS>`) and resume via the QoS-aware overload? **Clean-session state leaks on graceful DISCONNECT (dead-code cleanup).** As the PR body notes, `Disconnect` isn't dispatched by `MqttFactory`, so the cleanup added in `Disconnect.java:42-52` is unreachable today. Consequence: a clean-session client that disconnects gracefully leaves its `MqttSession` in `SessionRepository` until the same clientId next connects clean (`Connect.java` discards it lazily); clients that never reconnect leak indefinitely. Acceptable as a follow-up, but please either wire `Disconnect` dispatch in this PR or open a tracking issue and reference it in the PR/code so the leak isn't lost. Minor (pre-existing, now more reachable): `SubscribeRepository.add(List,List)` (L43-49) has a check-then-act race — `get(s)` returns `getOrDefault(topic, new CopyOnWriteArrayList<>())`, so for an absent topic two concurrent resumes can each create a fresh list and one `put` overwrites the other's channel. Not introduced here, just noting the resume path makes it reachable. -- 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]
