From d6e7f75a9021f42abadea36f3ada563e51ca756b Mon Sep 17 00:00:00 2001 From: Aniruddh Korde Date: Thu, 5 Mar 2026 21:04:39 +0530 Subject: [PATCH 1/5] Refactor ProxyWriter to use Cleaner instead of finalize --- .../java/hudson/remoting/ProxyWriter.java | 54 ++++++++++--------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/src/main/java/hudson/remoting/ProxyWriter.java b/src/main/java/hudson/remoting/ProxyWriter.java index 2da94fb51..1010975d9 100644 --- a/src/main/java/hudson/remoting/ProxyWriter.java +++ b/src/main/java/hudson/remoting/ProxyWriter.java @@ -30,6 +30,7 @@ import java.io.IOException; import java.io.InterruptedIOException; import java.io.Writer; +import java.lang.ref.Cleaner; import java.util.concurrent.ExecutionException; import java.util.logging.Level; import java.util.logging.Logger; @@ -40,6 +41,12 @@ * {@link Writer} on a remote machine. */ final class ProxyWriter extends Writer { + private static final Cleaner CLEANER = Cleaner.create(); + + @SuppressFBWarnings( + value = "URF_UNREAD_FIELD", + justification = "Cleaner registration must be kept strongly reachable") + private Cleaner.Cleanable cleanable; @GuardedBy("this") private Channel channel; @@ -88,7 +95,7 @@ synchronized void connect(@NonNull Channel channel, int oid) throws IOException } this.channel = channel; this.oid = oid; - + CLEANER.register(this, new CleanupTask(channel, oid)); window = channel.getPipeWindow(oid); // if we already have bytes to write, do so now. @@ -243,24 +250,6 @@ public void error(@CheckForNull Throwable cause) throws IOException { } } - @Override - // TODO: really? - @SuppressFBWarnings(value = "FI_FINALIZER_NULLS_FIELDS", justification = "As designed") - protected synchronized void finalize() throws Throwable { - super.finalize(); - // if we haven't done so, release the exported object on the remote side. - // if the object is auto-unexported, the export entry could have already been removed. - if (channel != null) { - if (channel.remoteCapability.supportsProxyWriter2_35()) { - channel.send(new Unexport(channel.newIoId(), oid)); - } else { - channel.send(new EOF(channel.newIoId(), oid)); - } - channel = null; - oid = -1; - } - } - /** * {@link Command} for sending bytes. */ @@ -412,11 +401,6 @@ protected void execute(final Channel channel) { } channel.pipeWriter.submit(ioId, () -> { channel.unexport(oid, createdAt, false); - try { - os.close(); - } catch (IOException e) { - // ignore errors - } }); } @@ -489,4 +473,24 @@ public String toString() { } private static final Logger LOGGER = Logger.getLogger(ProxyWriter.class.getName()); -} + + private static class CleanupTask implements Runnable { + private final Channel channel; + private final int oid; + + CleanupTask(Channel channel, int oid) { + this.channel = channel; + this.oid = oid; + } + + @Override + public void run() { + try { + // Sends the dead signal using all THREE required arguments + channel.send(new NotifyDeadWriter(channel, null, oid)); + } catch (Exception e) { + // Ignore errors during cleanup + } + } + } // Closes CleanupTask +} // Closes ProxyWriter (MUST BE THE VERY LAST LINE) \ No newline at end of file From 4daf1ea3c96d478ff57dff2685622076610a39ec Mon Sep 17 00:00:00 2001 From: Aniruddh Korde Date: Thu, 5 Mar 2026 21:30:35 +0530 Subject: [PATCH 2/5] Refactor ProxyOutputStream to use Cleaner instead of finalize --- .../hudson/remoting/ProxyOutputStream.java | 42 +++++++++++++------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/src/main/java/hudson/remoting/ProxyOutputStream.java b/src/main/java/hudson/remoting/ProxyOutputStream.java index 9e351ec55..5b821efaa 100644 --- a/src/main/java/hudson/remoting/ProxyOutputStream.java +++ b/src/main/java/hudson/remoting/ProxyOutputStream.java @@ -27,6 +27,7 @@ import java.io.IOException; import java.io.InterruptedIOException; import java.io.OutputStream; +import java.lang.ref.Cleaner; import java.util.concurrent.ExecutionException; import java.util.logging.Level; import java.util.logging.Logger; @@ -36,6 +37,9 @@ * {@link OutputStream} on a remote machine. */ final class ProxyOutputStream extends OutputStream implements ErrorPropagatingOutputStream { + private static final Cleaner CLEANER = Cleaner.create(); + private final Cleaner.Cleanable cleanable; + private Channel channel; private int oid; @@ -58,7 +62,9 @@ final class ProxyOutputStream extends OutputStream implements ErrorPropagatingOu * when it's {@link #connect(Channel,int) connected} later, * the data will be sent at once to the remote stream. */ - public ProxyOutputStream() {} + public ProxyOutputStream() { + this.cleanable = CLEANER.register(this, new CleanupTask(null, -1)); + } /** * Creates an already connected {@link ProxyOutputStream}. @@ -67,6 +73,7 @@ public ProxyOutputStream() {} * The object id of the exported {@link OutputStream}. */ public ProxyOutputStream(@NonNull Channel channel, int oid) throws IOException { + this.cleanable = CLEANER.register(this, new CleanupTask(channel, oid)); connect(channel, oid); } @@ -185,17 +192,6 @@ private void doClose(Throwable error) throws IOException { oid = -1; } - @Override - protected void finalize() throws Throwable { - super.finalize(); - // if we haven't done so, release the exported object on the remote side. - // if the object is auto-unexported, the export entry could have already been removed. - if (channel != null && oid != -1) { - channel.send(new Unexport(channel.newIoId(), oid)); - oid = -1; - } - } - /** * I/O operations in remoting gets executed by a separate pipe thread asynchronously. * So if a closure performs some I/O (such as writing to the RemoteOutputStream) then returns, @@ -465,4 +461,26 @@ public String toString() { } private static final Logger LOGGER = Logger.getLogger(ProxyOutputStream.class.getName()); + + private static class CleanupTask implements Runnable { + private final Channel channel; + private final int oid; + + CleanupTask(Channel channel, int oid) { + this.channel = channel; + this.oid = oid; + } + + @Override + public void run() { + if (channel != null && oid != -1) { + try { + channel.send(new Unexport(channel.newIoId(), oid)); + } catch (Exception e) { + java.util.logging.Logger.getLogger(ProxyOutputStream.class.getName()) + .log(java.util.logging.Level.FINE, "Failed to unexport during cleanup", e); + } + } + } + } } From 0aa4d7b0b397c4603531bf175de99499893efa77 Mon Sep 17 00:00:00 2001 From: Aniruddh Korde Date: Thu, 5 Mar 2026 21:31:54 +0530 Subject: [PATCH 3/5] Refactor ProxyOutputStream to use Cleaner --- .../hudson/remoting/FastPipedInputStream.java | 36 +++++++++++++++---- .../remoting/FastPipedOutputStream.java | 26 ++++++++++---- 2 files changed, 50 insertions(+), 12 deletions(-) diff --git a/src/main/java/hudson/remoting/FastPipedInputStream.java b/src/main/java/hudson/remoting/FastPipedInputStream.java index d36de2b33..c2ffc82dc 100644 --- a/src/main/java/hudson/remoting/FastPipedInputStream.java +++ b/src/main/java/hudson/remoting/FastPipedInputStream.java @@ -21,9 +21,11 @@ package hudson.remoting; import edu.umd.cs.findbugs.annotations.NonNull; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.io.IOException; import java.io.InputStream; import java.io.PipedInputStream; +import java.lang.ref.Cleaner; import java.lang.ref.WeakReference; /** @@ -38,6 +40,12 @@ * @see FastPipedOutputStream */ public class FastPipedInputStream extends InputStream { + private static final Cleaner CLEANER = Cleaner.create(); + + @SuppressFBWarnings( + value = "URF_UNREAD_FIELD", + justification = "Cleaner registration must be kept strongly reachable") + private final Cleaner.Cleanable cleanable; final byte[] buffer; /** @@ -58,6 +66,7 @@ public class FastPipedInputStream extends InputStream { */ public FastPipedInputStream() { this.buffer = new byte[0x10000]; + this.cleanable = CLEANER.register(this, new CleanupTask(this.buffer)); } /** @@ -79,6 +88,7 @@ public FastPipedInputStream(FastPipedOutputStream source, int bufferSize) throws connect(source); } this.buffer = new byte[bufferSize]; + this.cleanable = CLEANER.register(this, new CleanupTask(this.buffer)); } private void checkSource() throws IOException { @@ -130,12 +140,6 @@ public void connect(FastPipedOutputStream source) throws IOException { source.sink = new WeakReference<>(this); } - @Override - protected void finalize() throws Throwable { - super.finalize(); - close(); - } - @Override public void mark(int readLimit) {} @@ -210,4 +214,24 @@ static final class ClosedBy extends Throwable { super("The pipe was closed at...", error); } } + + private static class CleanupTask implements Runnable { + private final byte[] buffer; + + CleanupTask(byte[] buffer) { + this.buffer = buffer; + } + + @Override + @SuppressFBWarnings( + value = "NN_NAKED_NOTIFY", + justification = "Waking up threads during GC, state is irrelevant") + public void run() { + if (buffer != null) { + synchronized (buffer) { + buffer.notifyAll(); // Wake up any pending writers + } + } + } + } } diff --git a/src/main/java/hudson/remoting/FastPipedOutputStream.java b/src/main/java/hudson/remoting/FastPipedOutputStream.java index d70e8e3e5..73bdcfb1e 100644 --- a/src/main/java/hudson/remoting/FastPipedOutputStream.java +++ b/src/main/java/hudson/remoting/FastPipedOutputStream.java @@ -114,12 +114,6 @@ public void connect(FastPipedInputStream sink) throws IOException { sink.source = new WeakReference<>(this); } - @Override - protected void finalize() throws Throwable { - super.finalize(); - close(); - } - @Override @SuppressFBWarnings( value = "NN_NAKED_NOTIFY", @@ -202,4 +196,24 @@ public void write(@NonNull byte[] b, int off, int len) throws IOException { } static final int TIMEOUT = Integer.getInteger(FastPipedOutputStream.class.getName() + ".timeout", 10 * 1000); + + private static class CleanupTask implements Runnable { + private final FastPipedInputStream sink; + + CleanupTask(FastPipedInputStream sink) { + this.sink = sink; + } + + @Override + @SuppressFBWarnings( + value = "NN_NAKED_NOTIFY", + justification = "Waking up threads during GC, state is irrelevant") + public void run() { + if (sink != null && sink.buffer != null) { + synchronized (sink.buffer) { + sink.buffer.notifyAll(); // Wake up any pending readers + } + } + } + } } From e56faec931704f30b78154c5580cc3c647b884d3 Mon Sep 17 00:00:00 2001 From: Aniruddh Korde Date: Thu, 5 Mar 2026 23:35:31 +0530 Subject: [PATCH 4/5] Refactor FastPipedOutputStream to use Cleaner instead of finalize --- .../remoting/FastPipedOutputStream.java | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/src/main/java/hudson/remoting/FastPipedOutputStream.java b/src/main/java/hudson/remoting/FastPipedOutputStream.java index 73bdcfb1e..8bf8b38a4 100644 --- a/src/main/java/hudson/remoting/FastPipedOutputStream.java +++ b/src/main/java/hudson/remoting/FastPipedOutputStream.java @@ -26,6 +26,7 @@ import java.io.InterruptedIOException; import java.io.OutputStream; import java.io.PipedOutputStream; +import java.lang.ref.Cleaner; import java.lang.ref.WeakReference; /** @@ -40,7 +41,7 @@ * @see FastPipedOutputStream */ public class FastPipedOutputStream extends OutputStream implements ErrorPropagatingOutputStream { - + private static final Cleaner CLEANER = Cleaner.create(); WeakReference sink; private final Throwable allocatedAt = new Throwable(); @@ -112,6 +113,7 @@ public void connect(FastPipedInputStream sink) throws IOException { } this.sink = new WeakReference<>(sink); sink.source = new WeakReference<>(this); + CLEANER.register(this, new CleanupTask(this.sink)); } @Override @@ -197,23 +199,24 @@ public void write(@NonNull byte[] b, int off, int len) throws IOException { static final int TIMEOUT = Integer.getInteger(FastPipedOutputStream.class.getName() + ".timeout", 10 * 1000); - private static class CleanupTask implements Runnable { - private final FastPipedInputStream sink; + private static class CleanupTask implements Runnable { + private final WeakReference sinkRef; - CleanupTask(FastPipedInputStream sink) { - this.sink = sink; - } + CleanupTask(WeakReference sinkRef) { + this.sinkRef = sinkRef; + } - @Override - @SuppressFBWarnings( - value = "NN_NAKED_NOTIFY", - justification = "Waking up threads during GC, state is irrelevant") - public void run() { - if (sink != null && sink.buffer != null) { - synchronized (sink.buffer) { - sink.buffer.notifyAll(); // Wake up any pending readers + @Override + public void run() { + FastPipedInputStream s = sinkRef.get(); + if (s != null) { + synchronized (s.buffer) { + if (s.closed == null) { + s.closed = new FastPipedInputStream.ClosedBy(null); + s.buffer.notifyAll(); + } + } } } } - } } From be9518dcaae50d2cea172092424554fd7e2753a4 Mon Sep 17 00:00:00 2001 From: Aniruddh Korde Date: Fri, 6 Mar 2026 00:17:20 +0530 Subject: [PATCH 5/5] Fix SpotBugs and formatting violations --- .../remoting/FastPipedOutputStream.java | 28 +++++++++---------- .../java/hudson/remoting/ProxyWriter.java | 12 ++------ 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/src/main/java/hudson/remoting/FastPipedOutputStream.java b/src/main/java/hudson/remoting/FastPipedOutputStream.java index 8bf8b38a4..cb3cd1663 100644 --- a/src/main/java/hudson/remoting/FastPipedOutputStream.java +++ b/src/main/java/hudson/remoting/FastPipedOutputStream.java @@ -199,24 +199,24 @@ public void write(@NonNull byte[] b, int off, int len) throws IOException { static final int TIMEOUT = Integer.getInteger(FastPipedOutputStream.class.getName() + ".timeout", 10 * 1000); - private static class CleanupTask implements Runnable { - private final WeakReference sinkRef; + private static class CleanupTask implements Runnable { + private final WeakReference sinkRef; - CleanupTask(WeakReference sinkRef) { - this.sinkRef = sinkRef; - } + CleanupTask(WeakReference sinkRef) { + this.sinkRef = sinkRef; + } - @Override - public void run() { - FastPipedInputStream s = sinkRef.get(); - if (s != null) { - synchronized (s.buffer) { - if (s.closed == null) { - s.closed = new FastPipedInputStream.ClosedBy(null); - s.buffer.notifyAll(); - } + @Override + public void run() { + FastPipedInputStream s = sinkRef.get(); + if (s != null) { + synchronized (s.buffer) { + if (s.closed == null) { + s.closed = new FastPipedInputStream.ClosedBy(null); + s.buffer.notifyAll(); } } } } + } } diff --git a/src/main/java/hudson/remoting/ProxyWriter.java b/src/main/java/hudson/remoting/ProxyWriter.java index 1010975d9..04e844201 100644 --- a/src/main/java/hudson/remoting/ProxyWriter.java +++ b/src/main/java/hudson/remoting/ProxyWriter.java @@ -25,7 +25,6 @@ import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; -import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.io.CharArrayWriter; import java.io.IOException; import java.io.InterruptedIOException; @@ -43,11 +42,6 @@ final class ProxyWriter extends Writer { private static final Cleaner CLEANER = Cleaner.create(); - @SuppressFBWarnings( - value = "URF_UNREAD_FIELD", - justification = "Cleaner registration must be kept strongly reachable") - private Cleaner.Cleanable cleanable; - @GuardedBy("this") private Channel channel; @@ -488,9 +482,9 @@ public void run() { try { // Sends the dead signal using all THREE required arguments channel.send(new NotifyDeadWriter(channel, null, oid)); - } catch (Exception e) { - // Ignore errors during cleanup + } catch (IOException e) { + // ignore cleanup failures } } } // Closes CleanupTask -} // Closes ProxyWriter (MUST BE THE VERY LAST LINE) \ No newline at end of file +} // Closes ProxyWriter (MUST BE THE VERY LAST LINE)