Skip to content

Commit eaedae9

Browse files
jglickjglick
authored
Merge pull request #805 from jglick/System.exit
Avoid `System.exit` from `Launcher.run`
2 parents cdb9a45 + 68ab33f commit eaedae9

File tree

5 files changed

+155
-88
lines changed

5 files changed

+155
-88
lines changed

‎src/main/java/hudson/remoting/Engine.java‎

Lines changed: 105 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import jakarta.websocket.HandshakeResponse;
3535
import jakarta.websocket.Session;
3636
import jakarta.websocket.WebSocketContainer;
37+
import java.io.Closeable;
3738
import java.io.File;
3839
import java.io.FileInputStream;
3940
import java.io.FileNotFoundException;
@@ -84,6 +85,7 @@
8485
import org.glassfish.tyrus.client.ClientManager;
8586
import org.glassfish.tyrus.client.ClientProperties;
8687
import org.glassfish.tyrus.client.SslEngineConfigurator;
88+
import org.glassfish.tyrus.core.BaseContainer;
8789
import org.jenkinsci.remoting.engine.Jnlp4ConnectionState;
8890
import org.jenkinsci.remoting.engine.JnlpAgentEndpoint;
8991
import org.jenkinsci.remoting.engine.JnlpAgentEndpointConfigurator;
@@ -128,21 +130,7 @@ public class Engine extends Thread {
128130
/**
129131
* Thread pool that sets {@link #CURRENT}.
130132
*/
131-
private final ExecutorService executor = Executors.newCachedThreadPool(new ThreadFactory() {
132-
private final ThreadFactory defaultFactory = Executors.defaultThreadFactory();
133-
134-
@Override
135-
public Thread newThread(@NonNull final Runnable r) {
136-
Thread thread = defaultFactory.newThread(() -> {
137-
CURRENT.set(Engine.this);
138-
r.run();
139-
});
140-
thread.setDaemon(true);
141-
thread.setUncaughtExceptionHandler(
142-
(t, e) -> LOGGER.log(Level.SEVERE, e, () -> "Uncaught exception in thread " + t));
143-
return thread;
144-
}
145-
});
133+
private final ExecutorService executor;
146134

147135
/**
148136
* @deprecated
@@ -276,6 +264,7 @@ public Engine(
276264
String directConnection,
277265
String instanceIdentity,
278266
Set<String> protocols) {
267+
executor = makeExecutor(this, agentName);
279268
this.listener = listener;
280269
this.directConnection = directConnection;
281270
if (listener != null) {
@@ -308,6 +297,25 @@ private static URL ensureTrailingSlash(URL u) {
308297
}
309298
}
310299

300+
private static ExecutorService makeExecutor(Engine engine, String agentName) {
301+
return Executors.newCachedThreadPool(new ThreadFactory() {
302+
private final ThreadFactory defaultFactory =
303+
new NamingThreadFactory(Executors.defaultThreadFactory(), agentName);
304+
305+
@Override
306+
public Thread newThread(@NonNull final Runnable r) {
307+
Thread thread = defaultFactory.newThread(() -> {
308+
CURRENT.set(engine);
309+
r.run();
310+
});
311+
thread.setDaemon(true);
312+
thread.setUncaughtExceptionHandler(
313+
(t, e) -> LOGGER.log(Level.SEVERE, e, () -> "Uncaught exception in thread " + t));
314+
return thread;
315+
}
316+
});
317+
}
318+
311319
/**
312320
* Starts the engine.
313321
* The procedure initializes the working directory and all the required environment
@@ -538,12 +546,28 @@ public void removeListener(EngineListener el) {
538546
}
539547

540548
@Override
541-
@SuppressFBWarnings(value = "HARD_CODE_PASSWORD", justification = "Password doesn't need to be protected.")
542549
public void run() {
543-
if (webSocket) {
544-
runWebSocket();
545-
return;
550+
try {
551+
if (webSocket) {
552+
runWebSocket();
553+
} else {
554+
runTcp();
555+
}
556+
} finally {
557+
executor.shutdown(); // TODO Java 21 maybe .close()
558+
if (jarCache instanceof Closeable c) {
559+
try {
560+
c.close();
561+
} catch (IOException x) {
562+
LOGGER.log(Level.WARNING, null, x);
563+
}
564+
}
565+
events.completed();
546566
}
567+
}
568+
569+
@SuppressFBWarnings(value = "HARD_CODE_PASSWORD", justification = "Password doesn't need to be protected.")
570+
private void runTcp() {
547571
// Create the engine
548572
try {
549573
try (IOHub hub = IOHub.create(executor)) {
@@ -750,68 +774,73 @@ public void closeRead() throws IOException {
750774
SSLContext sslContext = getSSLContext(candidateCertificates, disableHttpsCertValidation);
751775
String wsUrl = hudsonUrl.toString().replaceFirst("^http", "ws");
752776
WebSocketContainer container = ContainerProvider.getWebSocketContainer();
753-
if (container instanceof ClientManager) {
754-
ClientManager client = (ClientManager) container;
755-
756-
String proxyHost = System.getProperty("http.proxyHost", System.getenv("proxy_host"));
757-
String proxyPort = System.getProperty("http.proxyPort");
758-
if (proxyHost != null
759-
&& "http".equals(hudsonUrl.getProtocol())
760-
&& NoProxyEvaluator.shouldProxy(hudsonUrl.getHost())) {
761-
URI proxyUri;
762-
if (proxyPort != null) {
763-
proxyUri = URI.create(String.format("http://%s:%s", proxyHost, proxyPort));
764-
} else {
765-
proxyUri = URI.create(String.format("http://%s", proxyHost));
766-
}
767-
client.getProperties().put(ClientProperties.PROXY_URI, proxyUri);
768-
if (proxyCredentials != null) {
769-
client.getProperties()
770-
.put(
771-
ClientProperties.PROXY_HEADERS,
772-
Map.of(
773-
"Proxy-Authorization",
774-
"Basic "
775-
+ Base64.getEncoder()
776-
.encodeToString(proxyCredentials.getBytes(
777-
StandardCharsets.UTF_8))));
777+
try {
778+
if (container instanceof ClientManager client) {
779+
780+
String proxyHost = System.getProperty("http.proxyHost", System.getenv("proxy_host"));
781+
String proxyPort = System.getProperty("http.proxyPort");
782+
if (proxyHost != null
783+
&& "http".equals(hudsonUrl.getProtocol())
784+
&& NoProxyEvaluator.shouldProxy(hudsonUrl.getHost())) {
785+
URI proxyUri;
786+
if (proxyPort != null) {
787+
proxyUri = URI.create(String.format("http://%s:%s", proxyHost, proxyPort));
788+
} else {
789+
proxyUri = URI.create(String.format("http://%s", proxyHost));
790+
}
791+
client.getProperties().put(ClientProperties.PROXY_URI, proxyUri);
792+
if (proxyCredentials != null) {
793+
client.getProperties()
794+
.put(
795+
ClientProperties.PROXY_HEADERS,
796+
Map.of(
797+
"Proxy-Authorization",
798+
"Basic "
799+
+ Base64.getEncoder()
800+
.encodeToString(proxyCredentials.getBytes(
801+
StandardCharsets.UTF_8))));
802+
}
778803
}
779-
}
780804

781-
if (sslContext != null) {
782-
SslEngineConfigurator sslEngineConfigurator = new SslEngineConfigurator(sslContext);
783-
if (hostnameVerifier != null) {
784-
sslEngineConfigurator.setHostnameVerifier(hostnameVerifier);
805+
if (sslContext != null) {
806+
SslEngineConfigurator sslEngineConfigurator = new SslEngineConfigurator(sslContext);
807+
if (hostnameVerifier != null) {
808+
sslEngineConfigurator.setHostnameVerifier(hostnameVerifier);
809+
}
810+
client.getProperties().put(ClientProperties.SSL_ENGINE_CONFIGURATOR, sslEngineConfigurator);
785811
}
786-
client.getProperties().put(ClientProperties.SSL_ENGINE_CONFIGURATOR, sslEngineConfigurator);
812+
}
813+
if (!succeedsWithRetries(() -> this.pingSuccessful(sslContext))) {
814+
return;
815+
}
816+
if (!succeedsWithRetries(() -> {
817+
container.connectToServer(
818+
new AgentEndpoint(),
819+
ClientEndpointConfig.Builder.create()
820+
.configurator(headerHandler)
821+
.build(),
822+
URI.create(wsUrl + "wsagents/"));
823+
return true;
824+
})) {
825+
return;
826+
}
827+
while (ch.get() == null) {
828+
Thread.sleep(100);
829+
}
830+
this.protocolName = "WebSocket";
831+
events.status("Connected");
832+
ch.get().join();
833+
events.status("Terminated");
834+
if (noReconnect) {
835+
return;
836+
}
837+
events.onDisconnect();
838+
reconnect();
839+
} finally {
840+
if (container instanceof BaseContainer baseContainer) {
841+
baseContainer.shutdown();
787842
}
788843
}
789-
if (!succeedsWithRetries(() -> this.pingSuccessful(sslContext))) {
790-
return;
791-
}
792-
if (!succeedsWithRetries(() -> {
793-
container.connectToServer(
794-
new AgentEndpoint(),
795-
ClientEndpointConfig.Builder.create()
796-
.configurator(headerHandler)
797-
.build(),
798-
URI.create(wsUrl + "wsagents/"));
799-
return true;
800-
})) {
801-
return;
802-
}
803-
while (ch.get() == null) {
804-
Thread.sleep(100);
805-
}
806-
this.protocolName = "WebSocket";
807-
events.status("Connected");
808-
ch.get().join();
809-
events.status("Terminated");
810-
if (noReconnect) {
811-
return;
812-
}
813-
events.onDisconnect();
814-
reconnect();
815844
}
816845
} catch (Exception e) {
817846
events.error(e);

‎src/main/java/hudson/remoting/EngineListener.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,4 +54,9 @@ default void onDisconnect() {}
5454
* @since 2.0
5555
*/
5656
default void onReconnect() {}
57+
58+
/**
59+
* Called when {@link Engine#run} exits, whether due to {@link #error} or normally.
60+
*/
61+
default void completed() {}
5762
}

‎src/main/java/hudson/remoting/EngineListenerSplitter.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,4 +54,11 @@ public void onReconnect() {
5454
l.onReconnect();
5555
}
5656
}
57+
58+
@Override
59+
public void completed() {
60+
for (EngineListener l : listeners) {
61+
l.completed();
62+
}
63+
}
5764
}

‎src/main/java/hudson/remoting/JarCacheSupport.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import edu.umd.cs.findbugs.annotations.NonNull;
44
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
5+
import java.io.Closeable;
56
import java.io.IOException;
67
import java.net.URL;
78
import java.util.concurrent.CompletableFuture;
@@ -21,7 +22,7 @@
2122
* @author Kohsuke Kawaguchi
2223
* @since 2.24
2324
*/
24-
public abstract class JarCacheSupport extends JarCache {
25+
public abstract class JarCacheSupport extends JarCache implements Closeable {
2526
/**
2627
* Remember in-progress jar file resolution to avoid retrieving the same jar file twice.
2728
*/
@@ -75,6 +76,12 @@ private CompletableFuture<URL> submitDownload(Channel channel, long sum1, long s
7576
return promise;
7677
}
7778

79+
@Override
80+
public void close() throws IOException {
81+
// TODO Java 21 maybe just use ExecutorService.close()
82+
downloader.shutdown();
83+
}
84+
7885
private class DownloadRunnable implements Runnable {
7986

8087
final Channel channel;

‎src/main/java/hudson/remoting/Launcher.java‎

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
import java.util.Locale;
5959
import java.util.Map;
6060
import java.util.Properties;
61+
import java.util.concurrent.ExecutionException;
6162
import java.util.concurrent.ExecutorService;
6263
import java.util.concurrent.Executors;
6364
import java.util.concurrent.TimeUnit;
@@ -77,6 +78,7 @@
7778
import org.jenkinsci.remoting.engine.WorkDirManager;
7879
import org.jenkinsci.remoting.util.DurationFormatter;
7980
import org.jenkinsci.remoting.util.PathUtils;
81+
import org.jenkinsci.remoting.util.SettableFuture;
8082
import org.jenkinsci.remoting.util.https.NoCheckHostnameVerifier;
8183
import org.kohsuke.args4j.Argument;
8284
import org.kohsuke.args4j.CmdLineException;
@@ -275,8 +277,7 @@ public void setAuth(String auth) {
275277
public void setConnectTo(String target) {
276278
String[] tokens = target.split(":");
277279
if (tokens.length != 2) {
278-
System.err.println("Illegal parameter: " + target);
279-
System.exit(1);
280+
throw new IllegalArgumentException(target);
280281
}
281282
connectionTarget = new InetSocketAddress(tokens[0], Integer.parseInt(tokens[1]));
282283
System.err.println(
@@ -723,11 +724,14 @@ private static void validateInboundAgentArgs(Launcher launcher) throws CmdLineEx
723724

724725
private void runAsInboundAgent() throws CmdLineException, IOException, InterruptedException {
725726
validateInboundAgentArgs(this);
726-
Engine engine = createEngine();
727+
SettableFuture<Void> completion = SettableFuture.create();
728+
Engine engine = createEngine(completion);
727729
engine.startEngine();
728730
try {
729-
engine.join();
731+
completion.get();
730732
LOGGER.fine("Engine has died");
733+
} catch (ExecutionException x) {
734+
throw new IllegalStateException(x.getCause());
731735
} finally {
732736
// if we are programmatically driven by other code, allow them to interrupt our blocking main thread to kill
733737
// the on-going connection to Jenkins
@@ -1109,10 +1113,16 @@ private static void closeWithLogOnly(Closeable stream, String name) {
11091113
}
11101114
}
11111115

1112-
private Engine createEngine() throws IOException {
1116+
private Engine createEngine(SettableFuture<Void> completion) throws IOException {
11131117
LOGGER.log(Level.INFO, "Setting up agent: {0}", name);
11141118
Engine engine = new Engine(
1115-
new CuiListener(), urls, secret, name, directConnection, instanceIdentity, new HashSet<>(protocols));
1119+
new CuiListener(completion),
1120+
urls,
1121+
secret,
1122+
name,
1123+
directConnection,
1124+
instanceIdentity,
1125+
new HashSet<>(protocols));
11161126
engine.setWebSocket(webSocket);
11171127
if (webSocketHeaders != null) {
11181128
engine.setWebSocketHeaders(webSocketHeaders);
@@ -1164,6 +1174,13 @@ private Engine createEngine() throws IOException {
11641174
* {@link EngineListener} implementation that sends output to {@link Logger}.
11651175
*/
11661176
private static final class CuiListener implements EngineListener {
1177+
1178+
private final SettableFuture<Void> completion;
1179+
1180+
CuiListener(SettableFuture<Void> completion) {
1181+
this.completion = completion;
1182+
}
1183+
11671184
@Override
11681185
public void status(String msg, Throwable t) {
11691186
LOGGER.log(Level.INFO, msg, t);
@@ -1175,12 +1192,14 @@ public void status(String msg) {
11751192
}
11761193

11771194
@Override
1178-
@SuppressFBWarnings(
1179-
value = "DM_EXIT",
1180-
justification = "Yes, we really want to exit in the case of severe error")
11811195
public void error(Throwable t) {
1182-
LOGGER.log(Level.SEVERE, t.getMessage(), t);
1183-
System.exit(-1);
1196+
LOGGER.log(Level.FINE, null, t); // thrown out of runAsInboundAgent so printed by caller
1197+
completion.setException(t);
1198+
}
1199+
1200+
@Override
1201+
public void completed() {
1202+
completion.set(null);
11841203
}
11851204
}
11861205

0 commit comments

Comments
 (0)