Fix #925 Memory Leak while WebSocketServerHandshakeException or Channel failed #926
Conversation
| } | ||
| } | ||
| try { | ||
| TimeUnit.SECONDS.sleep(3); |
There was a problem hiding this comment.
Can we avoid this sleep call? Don't want to have sleeping threads
| public ClientsBox() { | ||
| scheduledExecutorService.scheduleWithFixedDelay(() -> { | ||
| List<UUID> disconnected = new ArrayList<>(); | ||
| for (Map.Entry<UUID, ClientHead> entry : uuid2clients.entrySet()) { |
There was a problem hiding this comment.
Can you replace it with stream() ?
| private final ScheduledExecutorService scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(); | ||
|
|
||
| public ClientsBox() { | ||
| scheduledExecutorService.scheduleWithFixedDelay(() -> { |
There was a problem hiding this comment.
Why do we need a periodical task for this and schedule this check after client connection?
There was a problem hiding this comment.
I am afraid there would be bugs in future commit that cause this is not dereferenced after client disconnected
There was a problem hiding this comment.
like the one this PR fix, we cannot confirm there would not be any bugs alike in the future. but we should not let this kind of bug cause Memory Leak. We should actively detect that, remove that and warn it by log
There was a problem hiding this comment.
this logic should be implemented in com.corundumstudio.socketio.handler.ClientHead#onChannelDisconnect() method without ScheduledExecutorService usage.
There was a problem hiding this comment.
this logic should be implemented in com.corundumstudio.socketio.handler.ClientHead#onChannelDisconnect() method without ScheduledExecutorService usage.
but like the bug this time, onChannelDisconnect is called indeed(due to scheduled ping timeout), but the namespaceClients does not contain the client which throw WebSocketException, which result in the uuid2clients does not remove it.
There was a problem hiding this comment.
You're checking entry.getValue().isConnected() but it can be set only in onChannelDisconnect() handler. I suggest to call clientsBox.removeClient() in this method
| private final ScheduledExecutorService scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(); | ||
|
|
||
| public ClientsBox() { | ||
| scheduledExecutorService.scheduleWithFixedDelay(() -> { |
There was a problem hiding this comment.
this logic should be implemented in com.corundumstudio.socketio.handler.ClientHead#onChannelDisconnect() method without ScheduledExecutorService usage.
but like the bug this time, however, i cannot complain if you stick that this is not required, i can still remove it and keep the memory leak fixed, first thing first |
fix #925