AbstractUdpListener should be able to release its resources #140
Conversation
A new 'close' method has been added to AbstractUdpListener that is modeled after the 'close' implementation of AbstractTcpListener. This change can be used to prevent the issue described in jitsi#139
| //now clean up and exit | ||
| for (MySocket candidateSocket : sockets.values()) | ||
| { | ||
| candidateSocket.close(); |
There was a problem hiding this comment.
Have you tested this? Unless I'm missing something candidateSocket.close() will result in AbstractUdpListener.this.sockets.remove(remoteAddress); which clash with the for loop iterating over the map.
There was a problem hiding this comment.
I've not tested this, other than the unit test that I supplied. You're right, this might be causing ConcurrentModificationExceptions. Do you see a more elegant solution than creating a new collection?
| { | ||
| candidateSocket.close(); | ||
| } | ||
| socket.close(); |
There was a problem hiding this comment.
AbstractUdpListener.socket.close() is called here, and also in AbstractUdpListener.close(). Is there a reason?
There was a problem hiding this comment.
I've initially left it out here, but considered that the loop could also be broken due to an unexpected exception. That's why I put the call to socket.close() back in. If the socket is already closed, this second invocation is a no-op.
| import org.junit.Assert; | ||
| import org.junit.Test; | ||
|
|
||
| import java.net.BindException; |
There was a problem hiding this comment.
Thank you for adding tests!
Nits: license header, * imports
There was a problem hiding this comment.
Le sigh (pardon my French).
Part of the processing is removal from the collection that is being iterated over. To avoid concurrent modification exceptions, a copy of the collection is made, prior to iteration.
A new 'close' method has been added to AbstractUdpListener that is modeled after the 'close' implementation of AbstractTcpListener.
This change can be used to prevent the issue described in #139