Deprecating PermanentConnectionRefusalException

[jglick]

From this class’ Javadoc

no further connection attempts should be made

you would infer that throwing this error would produce different behavior compared to ConnectionRefusalException: rather than temporarily refusing a connection but letting the agent retry, the agent would immediately give up and exit. Yet it does not appear to actually do that, and from what I can tell going back to its introduction in #92 it never did.

First of all, no code actually uses this exception type currently.

remoting/src/main/java/org/jenkinsci/remoting/protocol/impl/ConnectionHeadersFilterLayer.java

Lines 378 to 379 in 3b293d5

	return abortCause instanceof PermanentConnectionRefusalException
	? new PermanentConnectionRefusalException(abortCause.getMessage())

is merely wrapping an exception already caught.

remoting/src/main/java/org/jenkinsci/remoting/protocol/impl/ConnectionHeadersFilterLayer.java

Lines 324 to 337 in 3b293d5

	if (response.startsWith("FATAL: ")) {
	String message = response.substring("FATAL: ".length());
	if (LOGGER.isLoggable(Level.WARNING)) {
	LOGGER.log(Level.WARNING, "[{0}] Local headers permanently rejected by remote: {1}",
	new Object[]{stack().name(), message});
	}
	if (LOGGER.isLoggable(Level.FINEST)) {
	LOGGER.log(Level.FINEST, "[{0}] Confirming receipt of permanently rejected connection: {1}",
	new Object[]{stack().name(), message});
	}
	next().doSend(ABORT_MESSAGE.duplicate());
	doStartAbort(new PermanentConnectionRefusalException(message), EMPTY_BUFFER);
	return;
	}

only runs on the agent side if the server side

remoting/src/main/java/org/jenkinsci/remoting/protocol/impl/ConnectionHeadersFilterLayer.java

Lines 225 to 226 in 3b293d5

	responseOutput = ByteBufferUtils.wrapUTF8(String.format("%s: %s",
	e instanceof PermanentConnectionRefusalException ? "FATAL" : "ERROR", e.getMessage()));

sent it. There are no known usages outside this repository.

So what happens if you try to use it?

diff --git src/main/java/org/jenkinsci/remoting/engine/JnlpProtocol4Handler.java src/main/java/org/jenkinsci/remoting/engine/JnlpProtocol4Handler.java
index 679d3ed0..323f27b2 100644
--- src/main/java/org/jenkinsci/remoting/engine/JnlpProtocol4Handler.java
+++ src/main/java/org/jenkinsci/remoting/engine/JnlpProtocol4Handler.java
@@ -58,6 +58,7 @@ import org.jenkinsci.remoting.protocol.impl.ChannelApplicationLayer;
 import org.jenkinsci.remoting.protocol.impl.ConnectionHeadersFilterLayer;
 import org.jenkinsci.remoting.protocol.impl.ConnectionRefusalException;
 import org.jenkinsci.remoting.protocol.impl.NIONetworkLayer;
+import org.jenkinsci.remoting.protocol.impl.PermanentConnectionRefusalException;
 import org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer;
 import org.jenkinsci.remoting.util.IOUtils;
 
@@ -291,7 +292,7 @@ public class JnlpProtocol4Handler extends JnlpProtocolHandler<Jnlp4ConnectionSta
             if (!client) {
                 String clientName = headers.get(JnlpConnectionState.CLIENT_NAME_KEY);
                 if (clientDatabase == null || !clientDatabase.exists(clientName)) {
-                    throw new ConnectionRefusalException("Unknown client name: " + clientName);
+                    throw new PermanentConnectionRefusalException("Unknown client name: " + clientName);
                 }
                 X509Certificate certificate = event.getCertificate();
                 JnlpClientDatabase.ValidationResult validation = certificate == null

looks like it would cause this error message to become fatal. Let us try it:

public final class UnknownClientNameTest {
    @Rule public final JenkinsRule r = new JenkinsRule();
    @Test public void run() throws Exception {
        System.out.println(new ProcessBuilder(
            "java",
            "-cp", Which.jarFile(Main.class).getAbsolutePath(),
            Main.class.getName(),
            "-url", r.getURL().toString(),
            "whatever",
            "nonexistent"
        ).inheritIO().start().waitFor());
    }
}

without the above patch (but with #675 reverted just in case, though it does not actually matter since that is about an earlier stage) prints among other things

INFO: [JNLP4-connect connection to localhost/127.0.0.1:…] Local headers refused by remote: Unknown client name: nonexistent

sleeps ten seconds then repeats, indefinitely. Switching to PermanentConnectionRefusalException changes the log message

WARNING: [JNLP4-connect connection to localhost/127.0.0.1:…] Local headers permanently rejected by remote: Unknown client name: nonexistent

but the behavior is identical otherwise: the agent keeps on running until you kill the test, repeating this warning every ten seconds.

remoting/src/test/java/org/jenkinsci/remoting/protocol/impl/ConnectionHeadersFilterLayerTest.java

Lines 169 to 170 in 3b293d5

	client.get().awaitClose();
	assertThat(client.get().getCloseCause(), instanceOf(PermanentConnectionRefusalException.class));

asserts that the “permanent” status is correctly transferred from server to client (I suppose via the ERROR vs. FATAL distinction). But all the test asserts is that the expected type of cause is preserved;

remoting/src/test/java/org/jenkinsci/remoting/protocol/impl/ConnectionHeadersFilterLayerTest.java

Lines 250 to 253 in 3b293d5

	client.get().awaitClose();
	assertThat(client.get().getCloseCause(),
	allOf(instanceOf(ConnectionRefusalException.class), not(instanceOf(PermanentConnectionRefusalException.class)))
	);

(“refuses” rather than “rejects”) also assumes that the connection was closed: it is not testing the higher-level behavior of the innerRun loop.

Since the class does not work as advertised, is not currently used, and probably would not be desirable even if it did work, I am deprecating it to avoid confusion.