From 444b80e3d093189e0591627cfc36462783e547d3 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 13 Sep 2023 14:03:19 -0400 Subject: [PATCH 1/3] [JENKINS-71970] Memory leak involving `BufferedBuildListener` --- .../workflow/log/BufferedBuildListener.java | 46 +++++++++++++++---- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/log/BufferedBuildListener.java b/src/main/java/org/jenkinsci/plugins/workflow/log/BufferedBuildListener.java index e3ab264d..33653904 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/log/BufferedBuildListener.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/log/BufferedBuildListener.java @@ -29,12 +29,15 @@ import hudson.remoting.Channel; import hudson.remoting.ChannelClosedException; import hudson.remoting.RemoteOutputStream; +import hudson.util.DaemonThreadFactory; +import hudson.util.NamingThreadFactory; import hudson.util.StreamTaskListener; import java.io.Closeable; import java.io.FilterOutputStream; import java.io.IOException; import java.io.OutputStream; import java.io.PrintStream; +import java.lang.ref.Cleaner; import java.util.logging.Logger; import org.jenkinsci.remoting.SerializableOnlyOverRemoting; @@ -46,10 +49,23 @@ final class BufferedBuildListener extends OutputStreamTaskListener.Default imple private static final Logger LOGGER = Logger.getLogger(BufferedBuildListener.class.getName()); + private static final Cleaner cleaner = Cleaner.create(new NamingThreadFactory(new DaemonThreadFactory(), BufferedBuildListener.class.getName() + ".cleaner")); + private final OutputStream out; + private final Channel channel; + private final Listener listener; BufferedBuildListener(OutputStream out) { this.out = out; + if (out instanceof CloseableOutputStream) { + channel = Channel.currentOrFail(); + listener = new Listener((CloseableOutputStream) out, channel); + channel.addListener(listener); + cleaner.register(this, listener); + } else { + channel = null; + listener = null; + } } @Override public OutputStream getOutputStream() { @@ -58,12 +74,32 @@ final class BufferedBuildListener extends OutputStreamTaskListener.Default imple @Override public void close() throws IOException { getLogger().close(); + if (listener != null) { + channel.removeListener(listener); + } } private Object writeReplace() { return new Replacement(this); } + private static final class Listener extends Channel.Listener implements Runnable { + private final CloseableOutputStream cos; + private final Channel channel; + Listener(CloseableOutputStream cos, Channel channel) { + this.cos = cos; + this.channel = channel; + } + @Override public void onClosed(Channel channel, IOException cause) { + LOGGER.fine(() -> "closing " + channel.getName()); + cos.close(channel, cause); + channel.removeListener(this); + } + @Override public void run() { + channel.removeListener(this); + } + } + private static final class Replacement implements SerializableOnlyOverRemoting { private static final long serialVersionUID = 1; @@ -76,15 +112,7 @@ private static final class Replacement implements SerializableOnlyOverRemoting { } private Object readResolve() { - var cos = new CloseableOutputStream(new GCFlushedOutputStream(new DelayBufferedOutputStream(ros, tuning))); - Channel.currentOrFail().addListener(new Channel.Listener() { - @Override public void onClosed(Channel channel, IOException cause) { - LOGGER.fine(() -> "closing " + channel.getName()); - cos.close(channel, cause); - channel.removeListener(this); - } - }); - return new BufferedBuildListener(cos); + return new BufferedBuildListener(new CloseableOutputStream(new GCFlushedOutputStream(new DelayBufferedOutputStream(ros, tuning)))); } } From acf5e4d2cdb1c8ed711fa512cd5fb5e41e72301d Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 13 Sep 2023 17:56:23 -0400 Subject: [PATCH 2/3] Mark fields `transient` to satisfy SpotBugs Co-authored-by: Devin Nusbaum --- .../jenkinsci/plugins/workflow/log/BufferedBuildListener.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/log/BufferedBuildListener.java b/src/main/java/org/jenkinsci/plugins/workflow/log/BufferedBuildListener.java index 33653904..47ca6e5f 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/log/BufferedBuildListener.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/log/BufferedBuildListener.java @@ -52,8 +52,8 @@ final class BufferedBuildListener extends OutputStreamTaskListener.Default imple private static final Cleaner cleaner = Cleaner.create(new NamingThreadFactory(new DaemonThreadFactory(), BufferedBuildListener.class.getName() + ".cleaner")); private final OutputStream out; - private final Channel channel; - private final Listener listener; + private transient final Channel channel; + private transient final Listener listener; BufferedBuildListener(OutputStream out) { this.out = out; From 34622bf731a546e9288dfe4ef927f24459f2c40c Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 13 Sep 2023 18:13:45 -0400 Subject: [PATCH 3/3] Add `serialVersionUID` to satisfy SpotBugs --- .../jenkinsci/plugins/workflow/log/BufferedBuildListener.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/log/BufferedBuildListener.java b/src/main/java/org/jenkinsci/plugins/workflow/log/BufferedBuildListener.java index 47ca6e5f..9d0dd81f 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/log/BufferedBuildListener.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/log/BufferedBuildListener.java @@ -47,6 +47,8 @@ */ final class BufferedBuildListener extends OutputStreamTaskListener.Default implements BuildListener, Closeable, SerializableOnlyOverRemoting { + private static final long serialVersionUID = 1; + private static final Logger LOGGER = Logger.getLogger(BufferedBuildListener.class.getName()); private static final Cleaner cleaner = Cleaner.create(new NamingThreadFactory(new DaemonThreadFactory(), BufferedBuildListener.class.getName() + ".cleaner"));