[JENKINS-35184] Servlet API dependent bits in a inner-class (#2551)

* [JENKINS-35184] Servlet API dependent bits in a inner-class This is in order to avoid loading ServletContextListener class from slaves classloader. * [JENKINS-35184] Don't use SystemProperties while initializing remote agents This rolls back the previous commit and introduces a new way to construct RingBufferLogHandler which avoids relying on SystemProperties to get the default size. * [JENKINS-35184] Mark SystemProperties as OnMaster only class Adding `OnMaster` annotation does not prevent the class from being loaded on remote agent but it gives a hint that this class should not be used on a remote agent. * [JENKINS-35184] Set SLAVE_LOG_HANDLER at the very beginning In the previous code cleaning existing log handlers, SLAVE_LOG_HANDLER is always null, as static fields are scoped by classloader.
jenkinsci · Sep 21, 2016 · 27d9b73 · 27d9b73
1 parent f191a3d
commit 27d9b73
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 7 deletions.
diff --git a/core/src/main/java/hudson/WebAppMain.java b/core/src/main/java/hudson/WebAppMain.java
@@ -77,7 +77,11 @@
  * @author Kohsuke Kawaguchi
  */
 public class WebAppMain implements ServletContextListener {
-    private final RingBufferLogHandler handler = new RingBufferLogHandler() {
+
+    // use RingBufferLogHandler class name to configure for backward compatibility
+    private static final int DEFAULT_RING_BUFFER_SIZE = SystemProperties.getInteger(RingBufferLogHandler.class.getName() + ".defaultSize", 256);
+
+    private final RingBufferLogHandler handler = new RingBufferLogHandler(DEFAULT_RING_BUFFER_SIZE) {
         @Override public synchronized void publish(LogRecord record) {
             if (record.getLevel().intValue() >= Level.INFO.intValue()) {
                 super.publish(record);

diff --git a/core/src/main/java/hudson/slaves/SlaveComputer.java b/core/src/main/java/hudson/slaves/SlaveComputer.java
@@ -56,6 +56,7 @@
 import jenkins.slaves.EncryptedSlaveAgentJnlpFile;
 import jenkins.slaves.JnlpSlaveAgentProtocol;
 import jenkins.slaves.systemInfo.SlaveSystemInfo;
+import jenkins.util.SystemProperties;
 import org.acegisecurity.context.SecurityContext;
 import org.acegisecurity.context.SecurityContextHolder;
 import org.kohsuke.stapler.HttpRedirect;
@@ -541,7 +542,7 @@ public void onClosed(Channel c, IOException cause) {
         // it'll have a catastrophic impact on the communication.
         channel.pinClassLoader(getClass().getClassLoader());
 
-        channel.call(new SlaveInitializer());
+        channel.call(new SlaveInitializer(DEFAULT_RING_BUFFER_SIZE));
         SecurityContext old = ACL.impersonate(ACL.SYSTEM);
         try {
             for (ComputerListener cl : ComputerListener.all()) {
@@ -804,11 +805,19 @@ static final class LogHolder {
         /**
          * This field is used on each agent to record logs on the agent.
          */
-        static final RingBufferLogHandler SLAVE_LOG_HANDLER = new RingBufferLogHandler();
+        static RingBufferLogHandler SLAVE_LOG_HANDLER;
     }
 
     private static class SlaveInitializer extends MasterToSlaveCallable<Void,RuntimeException> {
+        final int ringBufferSize;
+
+        public SlaveInitializer(int ringBufferSize) {
+            this.ringBufferSize = ringBufferSize;
+        }
+
         public Void call() {
+            SLAVE_LOG_HANDLER = new RingBufferLogHandler(ringBufferSize);
+
             // avoid double installation of the handler. JNLP slaves can reconnect to the master multiple times
             // and each connection gets a different RemoteClassLoader, so we need to evict them by class name,
             // not by their identity.
@@ -867,5 +876,8 @@ public List<LogRecord> call() {
         }
     }
 
+    // use RingBufferLogHandler class name to configure for backward compatibility
+    private static final int DEFAULT_RING_BUFFER_SIZE = SystemProperties.getInteger(RingBufferLogHandler.class.getName() + ".defaultSize", 256);
+
     private static final Logger LOGGER = Logger.getLogger(SlaveComputer.class.getName());
 }
diff --git a/core/src/main/java/hudson/util/RingBufferLogHandler.java b/core/src/main/java/hudson/util/RingBufferLogHandler.java
@@ -23,7 +23,6 @@
  */
 package hudson.util;
 
-import jenkins.util.SystemProperties;
 import java.util.AbstractList;
 import java.util.List;
 import java.util.logging.Handler;
@@ -36,12 +35,17 @@
  */
 public class RingBufferLogHandler extends Handler {
 
-    private static final int DEFAULT_RING_BUFFER_SIZE = SystemProperties.getInteger(RingBufferLogHandler.class.getName() + ".defaultSize", 256);
+    private static final int DEFAULT_RING_BUFFER_SIZE = Integer.getInteger(RingBufferLogHandler.class.getName() + ".defaultSize", 256);
 
     private int start = 0;
     private final LogRecord[] records;
     private volatile int size = 0;
 
+    /**
+     * This constructor is deprecated. It can't access system properties with {@link jenkins.util.SystemProperties}
+     * as it's not legal to use it on remoting agents.
+     */
+    @Deprecated
     public RingBufferLogHandler() {
         this(DEFAULT_RING_BUFFER_SIZE);
     }

diff --git a/core/src/main/java/jenkins/util/SystemProperties.java b/core/src/main/java/jenkins/util/SystemProperties.java
@@ -32,6 +32,8 @@
 import javax.servlet.ServletContext;
 import javax.servlet.ServletContextEvent;
 import javax.servlet.ServletContextListener;
+
+import jenkins.util.io.OnMaster;
 import org.apache.commons.lang.StringUtils;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
@@ -65,7 +67,7 @@
  */
 //TODO: Define a correct design of this engine later. Should be accessible in libs (remoting, stapler) and Jenkins modules too
 @Restricted(NoExternalUse.class)
-public class SystemProperties implements ServletContextListener {
+public class SystemProperties implements ServletContextListener, OnMaster {
     // this class implements ServletContextListener and is declared in WEB-INF/web.xml
 
     /**
@@ -88,7 +90,7 @@ public SystemProperties() {}
      * Called by the servlet container to initialize the {@link ServletContext}.
      */
     @Override
-    @SuppressFBWarnings(value = "ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD", 
+    @SuppressFBWarnings(value = "ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD",
             justification = "Currently Jenkins instance may have one ond only one context")
     public void contextInitialized(ServletContextEvent event) {
         theContext = event.getServletContext();