-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #925 Memory Leak while WebSocketServerHandshakeException or Channel failed #926
base: master
Are you sure you want to change the base?
Conversation
} | ||
} | ||
try { | ||
TimeUnit.SECONDS.sleep(3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid this sleep call? Don't want to have sleeping threads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok
public ClientsBox() { | ||
scheduledExecutorService.scheduleWithFixedDelay(() -> { | ||
List<UUID> disconnected = new ArrayList<>(); | ||
for (Map.Entry<UUID, ClientHead> entry : uuid2clients.entrySet()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you replace it with stream() ?
private final ScheduledExecutorService scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(); | ||
|
||
public ClientsBox() { | ||
scheduledExecutorService.scheduleWithFixedDelay(() -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a periodical task for this and schedule this check after client connection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic should be implemented in com.corundumstudio.socketio.handler.ClientHead#onChannelDisconnect() method without ScheduledExecutorService usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're checking entry.getValue().isConnected() but it can be set only in onChannelDisconnect() handler. I suggest to call clientsBox.removeClient() in this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HashZhang
private final ScheduledExecutorService scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(); | ||
|
||
public ClientsBox() { | ||
scheduledExecutorService.scheduleWithFixedDelay(() -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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