Skip to content

Commit b7902e2

Browse files
jzhugeclaude
andcommitted
sidecar: fix resource leak and state corruption in UdpWriter
Address additional bugs in the reconnect logic beyond the initial thread-safety fix: 1. **Resource leak**: If DatagramChannel.open() succeeds but connect() fails, the opened channel is never closed, causing file descriptor exhaustion. 2. **State corruption**: If connect() fails after opening the channel, this.channel points to an open but unconnected channel. Subsequent writes throw NotYetConnectedException (not caught by the ClosedChannelException handler), permanently breaking the writer. 3. **Data loss**: Original code dropped data after reconnection even if reconnection succeeded. **Changes:** - Use local variable in connect() and only assign on success - Close channel in connect() if connection fails (prevents leak) - Retry write after successful reconnection (prevents data loss) - Handle case where another thread already reconnected - Update test to verify data delivery after reconnection 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
1 parent 238c775 commit b7902e2

File tree

2 files changed

+29
-4
lines changed

2 files changed

+29
-4
lines changed

spectator-reg-sidecar/src/main/java/com/netflix/spectator/sidecar/UdpWriter.java

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,17 @@ final class UdpWriter extends SidecarWriter {
4444
private void connect() throws IOException {
4545
synchronized (lock) {
4646
DatagramChannel newChannel = DatagramChannel.open();
47-
newChannel.connect(address);
48-
channel = newChannel;
47+
try {
48+
newChannel.connect(address);
49+
channel = newChannel;
50+
} catch (Throwable t) {
51+
try {
52+
newChannel.close();
53+
} catch (IOException ignored) {
54+
// Suppress close exception during error handling
55+
}
56+
throw t;
57+
}
4958
}
5059
}
5160

@@ -60,12 +69,26 @@ private void connect() throws IOException {
6069
if (channel == ch) {
6170
try {
6271
connect();
72+
// After successful reconnection, retry the write once
73+
buffer.rewind();
74+
channel.write(buffer);
75+
return; // Write succeeded after reconnection
6376
} catch (IOException ex) {
6477
LOGGER.warn("channel closed, failed to reconnect", ex);
78+
throw ex;
79+
}
80+
} else {
81+
// Another thread reconnected, retry the write once with new channel
82+
try {
83+
buffer.rewind();
84+
channel.write(buffer);
85+
return; // Write succeeded with reconnected channel
86+
} catch (IOException ex) {
87+
LOGGER.warn("failed to write after reconnection by another thread", ex);
88+
throw ex;
6589
}
6690
}
6791
}
68-
throw e;
6992
}
7093
}
7194

spectator-reg-sidecar/src/test/java/com/netflix/spectator/sidecar/UdpWriterTest.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,11 @@ public void udpReconnectIfClosed() throws IOException {
4444
try (UdpServer server = new UdpServer()) {
4545
try (SidecarWriter w = SidecarWriter.create(server.address())) {
4646
// Used to simulate close from something like an interrupt. The next write
47-
// will fail and it should try to reconnect.
47+
// will trigger reconnection and retry the write.
4848
w.close();
4949
w.write("1");
50+
// After reconnection, the write should succeed (retry logic)
51+
Assertions.assertEquals("1", server.read());
5052

5153
w.write("2");
5254
Assertions.assertEquals("2", server.read());

0 commit comments

Comments
 (0)