uds-stream: close unused sockets #243
Conversation
We need to make sure we close the socket file descriptors before forgetting them otherwise we leak fds.
| @@ -136,9 +136,11 @@ private void connect() throws IOException { | |||
| } | |||
| if (!delegate.connect(address)) { | |||
| if (connectionTimeout > 0 && System.nanoTime() > deadline) { | |||
| delegate.close(); | |||
There was a problem hiding this comment.
Would it make sense to do this with a try/finally block? AFAICS setOption can throw too, and we'd leak then too.
There was a problem hiding this comment.
not sure how you would use a finally here 🤔 but I tried a try
There was a problem hiding this comment.
You are right, catch makes more sense of course.
| if (connectionTimeout > 0 && System.nanoTime() > deadline) { | ||
| delegate.close(); | ||
| throw new IOException("Connection timed out"); | ||
| try { |
There was a problem hiding this comment.
Would it make sense for this block to cover setOption for connectionTimeout above?
| throw new IOException("Connection timed out"); | ||
| } | ||
| if (!delegate.finishConnect()) { | ||
| closeSafe(delegate); |
There was a problem hiding this comment.
Is this call redundant with the catch block?
| @@ -1,5 +1,6 @@ | |||
| package com.timgroup.statsd; | |||
|
|
|||
| import java.net.SocketOption; | |||
There was a problem hiding this comment.
This appears to be unused.
|
/merge |
|
🚂 MergeQueue Pull request added to the queue. This build is going to start soon! (estimated merge in less than 0s) Use |
We need to make sure we close the socket
file descriptors before forgetting them otherwise
we leak fds.