remove-disconnected-clients #36
Conversation
…o remove-disconnected-clients
| while (iterator.hasNext()) { | ||
| Socket client = iterator.next(); | ||
| if (client.isClosed()) { | ||
| System.out.println("Client disconnected: " + client.getInetAddress().getHostName()); |
There was a problem hiding this comment.
Great job! I think your methods look good. Consider using logger.info(Log4j) instead of System.out.println. It provides better flexibility for handling log messages and is more advantageous in larger projects.
There was a problem hiding this comment.
good suggestion, i will look into this!
There was a problem hiding this comment.
If we want to introduce logging using a logging framework like Log4j I suggest that we create an issue for that. Because using a framework means we will have dependencies in runtime that must be included in our Dockerfile so changes must be made there also.
Unless we use the built in logging from java.util.logging.
Pros/Cons? https://www.loggly.com/ultimate-guide/java-logging-basics/
There was a problem hiding this comment.
Aren't we missing detection of DISCONNECT messages from the client that will be sent to our server before closing the connection? This code handles removal of clients that are forcefully disconnected and the Socket is closed? But we must also handle graceful disconnects.
Might also be time to introduce some sort of Client object to store in the list since the Socket reference is only a part of the information we need to store for a client? We also might need to store clientId, a list of Topics that the client is subscribed to and other status information.
There was a problem hiding this comment.
correct this method only handles when a client is forcefully disconnected
There was a problem hiding this comment.
It looks good, but doesnt solve disconnects should be fine since there is an #18 being worked on
There was a problem hiding this comment.
It looks good, but doesnt solve disconnects should be fine since there is an #18 being worked on
Proposed Changes
Remove disconnected clients
Description
Added a method that removes disconnected clients from the clients list