Merge pull request #7650 from NotMyFault/backporting-2.387.1

Backporting for 2.387.1

core/src/main/java/hudson/TcpSlaveAgentListener.java

@@ -26,7 +26,6 @@ package hudson;
 
 import edu.umd.cs.findbugs.annotations.Nullable;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
-import hudson.model.AperiodicWork;
 import hudson.slaves.OfflineCause;
 import hudson.util.VersionNumber;
 import java.io.ByteArrayInputStream;
@@ -81,7 +80,7 @@ import org.kohsuke.accmod.restrictions.NoExternalUse;
 @StaplerAccessibleType
 public final class TcpSlaveAgentListener extends Thread {
 
-    private final ServerSocketChannel serverSocket;
+    private ServerSocketChannel serverSocket;
     private volatile boolean shuttingDown;
 
     public final int configuredPort;
@@ -92,24 +91,27 @@ public final class TcpSlaveAgentListener extends Thread {
      */
     public TcpSlaveAgentListener(int port) throws IOException {
         super("TCP agent listener port=" + port);
-        try {
-            serverSocket = ServerSocketChannel.open();
-            serverSocket.socket().bind(new InetSocketAddress(port));
-        } catch (BindException e) {
-            throw (BindException) new BindException("Failed to listen on port " + port + " because it's already in use.").initCause(e);
-        }
+        serverSocket = createSocket(port);
         this.configuredPort = port;
         setUncaughtExceptionHandler((t, e) -> {
-            LOGGER.log(Level.SEVERE, "Uncaught exception in TcpSlaveAgentListener " + t + ", attempting to reschedule thread", e);
+            LOGGER.log(Level.SEVERE, "Uncaught exception in TcpSlaveAgentListener " + t, e);
             shutdown();
-            TcpSlaveAgentListenerRescheduler.schedule(t, e);
         });
-
         LOGGER.log(Level.FINE, "TCP agent listener started on port {0}", getPort());
-
         start();
     }
 
+    private static ServerSocketChannel createSocket(int port) throws IOException {
+        ServerSocketChannel result;
+        try {
+            result = ServerSocketChannel.open();
+            result.socket().bind(new InetSocketAddress(port));
+        } catch (BindException e) {
+            throw (BindException) new BindException("Failed to listen on port " + port + " because it's already in use.").initCause(e);
+        }
+        return result;
+    }
+
     /**
      * Gets the TCP port number in which we are listening.
      */
@@ -172,9 +174,9 @@ public final class TcpSlaveAgentListener extends Thread {
 
     @Override
     public void run() {
-        try {
-            // the loop eventually terminates when the socket is closed.
-            while (!shuttingDown) {
+        // the loop eventually terminates when the thread shuts down
+        while (!shuttingDown) {
+            try {
                 Socket s = serverSocket.accept().socket();
 
                 // this prevents a connection from silently terminated by the router in between or the other peer
@@ -184,18 +186,20 @@ public final class TcpSlaveAgentListener extends Thread {
                 // we take care of buffering on our own
                 s.setTcpNoDelay(true);
 
-                new ConnectionHandler(s, new ConnectionHandlerFailureCallback(this) {
-                    @Override
-                    public void run(Throwable cause) {
-                        LOGGER.log(Level.WARNING, "Connection handler failed, restarting listener", cause);
-                        shutdown();
-                        TcpSlaveAgentListenerRescheduler.schedule(getParentThread(), cause);
+                new ConnectionHandler(s).start();
+            } catch (Throwable e) {
+                if (!shuttingDown) {
+                    LOGGER.log(Level.SEVERE, "Failed to accept TCP connections", e);
+                    if (!serverSocket.isOpen()) {
+                        LOGGER.log(Level.INFO, "Restarting server socket");
+                        try {
+                            serverSocket = createSocket(configuredPort);
+                            LOGGER.log(Level.INFO, "TCP agent listener restarted on port {0}", getPort());
+                        } catch (IOException ioe) {
+                            LOGGER.log(Level.WARNING, "Failed to restart server socket", ioe);
+                        }
                     }
-                }).start();
-            }
-        } catch (IOException e) {
-            if (!shuttingDown) {
-                LOGGER.log(Level.SEVERE, "Failed to accept TCP connections", e);
+                }
             }
         }
     }
@@ -234,21 +238,12 @@ public final class TcpSlaveAgentListener extends Thread {
          */
         private final int id;
 
-        ConnectionHandler(Socket s, ConnectionHandlerFailureCallback parentTerminator) {
+        ConnectionHandler(Socket s) {
             this.s = s;
             synchronized (getClass()) {
                 id = iotaGen++;
             }
             setName("TCP agent connection handler #" + id + " with " + s.getRemoteSocketAddress());
-            setUncaughtExceptionHandler((t, e) -> {
-                LOGGER.log(Level.SEVERE, "Uncaught exception in TcpSlaveAgentListener ConnectionHandler " + t, e);
-                try {
-                    s.close();
-                    parentTerminator.run(e);
-                } catch (IOException e1) {
-                    LOGGER.log(Level.WARNING, "Could not close socket after unexpected thread death", e1);
-                }
-            });
         }
 
         @Override
@@ -295,7 +290,7 @@ public final class TcpSlaveAgentListener extends Thread {
                 } catch (IOException ex) {
                     // try to clean up the socket
                 }
-            } catch (IOException e) {
+            } catch (Throwable e) {
                 if (e instanceof EOFException) {
                     LOGGER.log(Level.INFO, () -> "Connection " + connectionInfo + " failed: " + e.getMessage());
                 } else {
@@ -351,21 +346,6 @@ public final class TcpSlaveAgentListener extends Thread {
         }
     }
 
-    // This is essentially just to be able to pass the parent thread into the callback, as it can't access it otherwise
-    private abstract static class ConnectionHandlerFailureCallback {
-        private Thread parentThread;
-
-        ConnectionHandlerFailureCallback(Thread parentThread) {
-            this.parentThread = parentThread;
-        }
-
-        public Thread getParentThread() {
-            return parentThread;
-        }
-
-        public abstract void run(Throwable cause);
-    }
-
     /**
      * This extension provides a Ping protocol that allows people to verify that the {@link TcpSlaveAgentListener} is alive.
      * We also use this to wake the acceptor thread on termination.
@@ -436,83 +416,6 @@ public final class TcpSlaveAgentListener extends Thread {
         }
     }
 
-    /**
-     * Reschedules the {@code TcpSlaveAgentListener} on demand.  Disables itself after running.
-     */
-    @Extension
-    @Restricted(NoExternalUse.class)
-    public static class TcpSlaveAgentListenerRescheduler extends AperiodicWork {
-        private Thread originThread;
-        private Throwable cause;
-        private long recurrencePeriod = 5000;
-        private boolean isActive;
-
-        public TcpSlaveAgentListenerRescheduler() {
-            isActive = false;
-        }
-
-        public TcpSlaveAgentListenerRescheduler(Thread originThread, Throwable cause) {
-            this.originThread = originThread;
-            this.cause = cause;
-            this.isActive = false;
-        }
-
-        public void setOriginThread(Thread originThread) {
-            this.originThread = originThread;
-        }
-
-        public void setCause(Throwable cause) {
-            this.cause = cause;
-        }
-
-        public void setActive(boolean active) {
-            isActive = active;
-        }
-
-        @Override
-        public long getRecurrencePeriod() {
-            return recurrencePeriod;
-        }
-
-        @Override
-        public AperiodicWork getNewInstance() {
-            return new TcpSlaveAgentListenerRescheduler(originThread, cause);
-        }
-
-        @Override
-        protected void doAperiodicRun() {
-            if (isActive) {
-                try {
-                    if (originThread.isAlive()) {
-                        originThread.interrupt();
-                    }
-                    int port = Jenkins.get().getSlaveAgentPort();
-                    if (port != -1) {
-                        new TcpSlaveAgentListener(port).start();
-                        LOGGER.log(Level.INFO, "Restarted TcpSlaveAgentListener");
-                    } else {
-                        LOGGER.log(Level.SEVERE, "Uncaught exception in TcpSlaveAgentListener " + originThread + ". Port is disabled, not rescheduling", cause);
-                    }
-                    isActive = false;
-                } catch (IOException e) {
-                    LOGGER.log(Level.SEVERE, "Could not reschedule TcpSlaveAgentListener - trying again.", cause);
-                }
-            }
-        }
-
-        public static void schedule(Thread originThread, Throwable cause) {
-            schedule(originThread, cause, 5000);
-        }
-
-        public static void schedule(Thread originThread, Throwable cause, long approxDelay) {
-            TcpSlaveAgentListenerRescheduler rescheduler = AperiodicWork.all().get(TcpSlaveAgentListenerRescheduler.class);
-            rescheduler.originThread = originThread;
-            rescheduler.cause = cause;
-            rescheduler.recurrencePeriod = approxDelay;
-            rescheduler.isActive = true;
-        }
-    }
-
 
     /**
      * Connection terminated because we are reconnected from the current peer.

core/src/main/java/jenkins/agents/WebSocketAgents.java

@@ -44,6 +44,7 @@ import java.security.MessageDigest;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.TimeUnit;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import jenkins.slaves.JnlpAgentReceiver;
@@ -166,11 +167,19 @@ public final class WebSocketAgents extends InvisibleAction implements Unprotecte
 
         class Transport extends AbstractByteBufferCommandTransport {
 
+            Transport() {
+                super(true);
+            }
+
             @Override
-            protected void write(ByteBuffer header, ByteBuffer data) throws IOException {
-                LOGGER.finest(() -> "sending message of length " + ChunkHeader.length(ChunkHeader.peek(header)));
-                sendBinary(header, false);
-                sendBinary(data, true);
+            protected void write(ByteBuffer headerAndData) throws IOException {
+                // As in Engine.runWebSocket:
+                LOGGER.finest(() -> "sending message of length " + (headerAndData.remaining() - ChunkHeader.SIZE));
+                try {
+                    sendBinary(headerAndData).get(5, TimeUnit.MINUTES);
+                } catch (Exception x) {
+                    throw new IOException(x);
+                }
             }
 
             @Override

core/src/main/java/jenkins/telemetry/Telemetry.java

@@ -200,7 +200,7 @@ public abstract class Telemetry implements ExtensionPoint {
                     return;
                 }
 
-                JSONObject data = new JSONObject();
+                JSONObject data = null;
                 try {
                     data = telemetry.createContent();
                 } catch (RuntimeException e) {

core/src/main/resources/hudson/model/Computer/index-top.jelly

 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:l="/lib/layout" xmlns:t="/lib/hudson">
-  <!-- Node name and status -->
-  <h1>
-    <l:icon src="${it.iconClassName}" class="icon-xlg" />
-    ${it.caption}
-  </h1>
-
   <!-- Node description -->
   <t:editableDescription permission="${it.CONFIGURE}" description="${it.node.nodeDescription}" submissionUrl="submitDescription" />
 </j:jelly>

core/src/main/resources/hudson/model/Computer/index.jelly

@@ -28,31 +28,34 @@ THE SOFTWARE.
     <st:include page="sidepanel.jelly" />
     <!-- no need for additional breadcrumb here as we're on an index page already including breadcrumb -->
     <l:main-panel>
-      <st:include page="index-top.jelly" it="${it}" />
-
-      <!-- temporarily offline switch -->
-      <div style="float:right">
+      <l:app-bar title="${it.caption}">
+        <j:if test="${it.manualLaunchAllowed}">
+          <st:include from="${it.launcher}" page="app-bar-controls.jelly" optional="true"/>
+        </j:if>
+        <!-- temporarily offline switch -->
         <j:choose>
           <j:when test="${it.temporarilyOffline}">
             <l:hasPermission permission="${it.CONNECT}">
               <form method="post" action="toggleOffline">
                 <f:submit value="${%submit.temporarilyOffline}"  />
               </form>
-              <br/>
               <form method="post" action="setOfflineCause">
-              <f:submit value="${%submit.updateOfflineCause}"  />
+              <f:submit primary="false" value="${%submit.updateOfflineCause}"  />
               </form>
             </l:hasPermission>
           </j:when>
           <j:otherwise>
             <l:hasPermission permission="${it.DISCONNECT}">
               <form method="post" action="markOffline">
-                <f:submit value="${%submit.not.temporarilyOffline}"  />
+                <f:submit primary="false" value="${%submit.not.temporarilyOffline}"  />
               </form>
             </l:hasPermission>
           </j:otherwise>
         </j:choose>
-      </div>
+        <t:help href="https://www.jenkins.io/doc/book/using/using-agents/" />
+      </l:app-bar>
+
+      <st:include page="index-top.jelly" it="${it}" />
 
       <j:if test="${it.offlineCause!=null and it.offline and !it.connecting}">
         <st:include it="${it.offlineCause}" page="cause.jelly" />
@@ -67,7 +70,7 @@ THE SOFTWARE.
       </j:if>
 
         <j:if test="${it.node.assignedLabels.size() gt 1}">
-        <div>
+        <div class="jenkins-!-margin-bottom-3">
           <h2>${%Labels}</h2>
           <j:forEach var="entry" items="${it.node.labelCloud}">
             <!-- Skip the label for this node -->

core/src/main/resources/hudson/model/Computer/sidepanel.jelly

@@ -30,7 +30,7 @@ THE SOFTWARE.
   <l:header />
   <l:side-panel>
     <l:tasks>
-      <l:task contextMenu="false" href="${rootURL}/${it.url}" icon="symbol-computer" title="${%Status}"/>
+      <l:task contextMenu="false" href="${rootURL}/${it.url}" icon="${it.iconClassName}" title="${%Status}"/>
       <l:task href="${rootURL}/${it.url}delete" icon="icon-edit-delete icon-md" permission="${it.DELETE}" title="${%Delete Agent}"/>
       <l:task href="${rootURL}/${it.url}configure" icon="symbol-settings" permission="${it.EXTENDED_READ}"
               title="${it.hasPermission(it.CONFIGURE) ? '%Configure' : '%View Configuration'}"/>

core/src/main/resources/hudson/slaves/ComputerLauncher/app-bar-controls.jelly

+<!--
+The MIT License
+
+Copyright (c) 2023, Jenkins project contributors
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in
+all copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+THE SOFTWARE.
+-->
+
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:l="/lib/layout" xmlns:f="/lib/form">
+  <j:if test="${it.launchSupported and it.offline and !it.temporarilyOffline}">
+    <j:choose>
+      <j:when test="${it.isConnecting()}">
+        <l:hasPermission permission="${it.CONNECT}">
+          <form method="post" action="launchSlaveAgent">
+            <f:submit value="${%Relaunch agent}"/>
+          </form>
+        </l:hasPermission>
+      </j:when>
+      <j:otherwise>
+        <l:hasPermission permission="${it.CONNECT}">
+          <form method="post" action="launchSlaveAgent">
+            <f:submit value="${%Launch agent}"/>
+          </form>
+        </l:hasPermission>
+      </j:otherwise>
+    </j:choose>
+  </j:if>
+</j:jelly>
\ No newline at end of file

core/src/main/resources/hudson/slaves/ComputerLauncher/main.jelly

 <!--
 The MIT License
 
-Copyright (c) 2004-2009, Sun Microsystems, Inc., Kohsuke Kawaguchi
+Copyright (c) 2004-2023, Sun Microsystems, Inc., Kohsuke Kawaguchi, Jenkins project contributors
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
 of this software and associated documentation files (the "Software"), to deal
@@ -23,31 +23,13 @@ THE SOFTWARE.
 -->
 
 <?jelly escape-by-default='true'?>
-<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form" xmlns:i="jelly:fmt">
-  <j:if test="${it.launchSupported and it.offline and !it.temporarilyOffline}">
-    <j:choose>
-      <j:when test="${it.isConnecting()}">
-        <p>
-          ${%launchingDescription}
-          <j:if test="${it.hasPermission(it.CONFIGURE) or it.hasPermission(it.CONNECT)}">
-          <a href="log">${%See log for more details}</a>
-          </j:if>
-        </p>
-
-        <l:hasPermission permission="${it.CONNECT}">
-          <form method="post" action="launchSlaveAgent">
-            <f:submit value="${%Relaunch agent}" />
-          </form>
-        </l:hasPermission>
-      </j:when>
-
-      <j:otherwise>
-        <l:hasPermission permission="${it.CONNECT}">
-          <form method="post" action="launchSlaveAgent">
-            <f:submit value="${%Launch agent}" />
-          </form>
-        </l:hasPermission>
-      </j:otherwise>
-    </j:choose>
+<j:jelly xmlns:j="jelly:core">
+  <j:if test="${it.launchSupported and it.offline and !it.temporarilyOffline and it.isConnecting()}">
+    <p>
+      ${%launchingDescription}
+      <j:if test="${it.hasPermission(it.CONFIGURE) or it.hasPermission(it.CONNECT)}">
+        <a href="log">${%See log for more details}</a>
+      </j:if>
+    </p>
   </j:if>
 </j:jelly>

core/src/main/resources/jenkins/model/Jenkins/MasterComputer/index-top.jelly

 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:l="/lib/layout">
-  <!-- Node name and status -->
-  <h1>
-    <l:icon src="${it.iconClassName}" class="icon-xlg" />
-    ${it.caption}
-  </h1>
-
   <!-- Node description -->
   <div class="jenkins-!-margin-bottom-5">
     ${%blurb}

pom.xml

@@ -87,7 +87,7 @@ THE SOFTWARE.
     <changelog.url>https://www.jenkins.io/changelog</changelog.url>
 
     <!-- Bundled Remoting version -->
-    <remoting.version>3085.vc4c6977c075a</remoting.version>
+    <remoting.version>3107.v665000b_51092</remoting.version>
     <!-- Minimum Remoting version, which is tested for API compatibility -->
     <remoting.minimum.supported.version>4.7</remoting.minimum.supported.version>
 

test/pom.xml

@@ -78,7 +78,7 @@ THE SOFTWARE.
         <!-- requireUpperBoundDeps via matrix-project and junit -->
         <groupId>org.jenkins-ci.plugins</groupId>
         <artifactId>script-security</artifactId>
-        <version>1228.vd93135a_2fb_25</version>
+        <version>1229.v4880b_b_e905a_6</version>
       </dependency>
     </dependencies>
   </dependencyManagement>

test/src/test/java/jenkins/telemetry/TelemetryTest.java

@@ -5,6 +5,8 @@ import static org.hamcrest.CoreMatchers.hasItem;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.empty;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
@@ -101,6 +103,55 @@ public class TelemetryTest {
         assertThat(correlators, hasItem(DigestUtils.sha256Hex(correlationId + "test-data")));
     }
 
+    @Test
+    public void testNonSubmissionOnError() throws Exception {
+        assertEquals("no requests received", 0, counter);
+        ExtensionList.lookupSingleton(Telemetry.TelemetryReporter.class).doRun();
+        await().pollInterval(250, TimeUnit.MILLISECONDS)
+                .atMost(10, TimeUnit.SECONDS)
+                .until(logger::getMessages, hasItem("Failed to build telemetry content for: 'throwing'"));
+        await().pollInterval(250, TimeUnit.MILLISECONDS)
+                .atMost(2, TimeUnit.SECONDS)
+                .until(logger::getMessages, hasItem("Skipping telemetry for 'throwing' as it has no data"));
+        await().pollInterval(250, TimeUnit.MILLISECONDS)
+                .atMost(10, TimeUnit.SECONDS)
+                .until(() -> types, is(not(empty())));
+        assertThat(types, not(contains("throwing")));
+    }
+
+    @TestExtension("testNonSubmissionOnError")
+    public static class ExceptionThrowingTelemetry extends Telemetry {
+
+        @NonNull
+        @Override
+        public String getDisplayName() {
+            return "throwing";
+        }
+
+        @NonNull
+        @Override
+        public String getId() {
+            return "throwing";
+        }
+
+        @NonNull
+        @Override
+        public LocalDate getStart() {
+            return LocalDate.MIN;
+        }
+
+        @NonNull
+        @Override
+        public LocalDate getEnd() {
+            return LocalDate.MAX;
+        }
+
+        @Override
+        public JSONObject createContent() {
+            throw new RuntimeException("something went wrong");
+        }
+    }
+
     @TestExtension
     public static class EmptyTelemetry extends Telemetry {
 

war/pom.xml

@@ -555,14 +555,14 @@ THE SOFTWARE.
                   <!-- dependency of mina-sshd-api-core -->
                   <groupId>io.jenkins.plugins.mina-sshd-api</groupId>
                   <artifactId>mina-sshd-api-common</artifactId>
-                  <version>2.9.1-44.v476733c11f82</version>
+                  <version>2.9.2-50.va_0e1f42659a_a</version>
                   <type>hpi</type>
                 </artifactItem>
                 <artifactItem>
                   <!-- dependency of sshd -->
                   <groupId>io.jenkins.plugins.mina-sshd-api</groupId>
                   <artifactId>mina-sshd-api-core</artifactId>
-                  <version>2.9.1-44.v476733c11f82</version>
+                  <version>2.9.2-50.va_0e1f42659a_a</version>
                   <type>hpi</type>
                 </artifactItem>
               </artifactItems>