Aias00 commented on code in PR #7343:
URL: https://github.com/apache/shenyu/pull/7343#discussion_r4128698827
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/listener/websocket/WebsocketCollector.java:
##########
@@ -413,23 +475,84 @@ private void sendNext() {
sending = false;
return;
}
+ inFlightMessage = message;
}
+ final ScheduledFuture<?> future = SEND_WATCHDOG.schedule(
Review Comment:
`timeoutFuture` is never assigned anywhere — `sendNext()` keeps the
`ScheduledFuture` in a local variable and passes it to `onSendResult`, so the
field stays `null` forever and both cancel blocks (`forceClose():533-536` and
`close():552-555`) are dead code.
It is not a correctness problem today: `onSendTimeout()` re-checks `closed`
first, so a task that outlives its session returns immediately. But it does
mean every session closed while a message is in flight keeps its watchdog task
(and a strong reference to this queue and the `Session`) alive for up to
`sendTimeoutMillis` (30s by default), on a scheduler that is never drained.
Either assign it where the send is scheduled, i.e. inside the `synchronized`
block that sets `inFlightMessage`:
```java
inFlightMessage = message;
// then, after scheduling:
timeoutFuture = future;
```
or drop the field and the two cancel blocks and say explicitly that `closed`
is the only guard. As written it reads like the timeout is cancelled on close
when it is not.
--
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]
