Introducing `ControllerToAgentCallable` and `ControllerToAgentFileCallable` #9921

jglick · 2024-10-28T15:26:48Z

While updating some plugin code to take advantage of Java 17 records, I noticed that one major use case for a static final class with all final fields was MasterToSlaveCallables. Unfortunately these cannot be converted to record since that cannot extend another class. I thought it would be nicer to introduce a similar interface (with updated naming, while we are here!) that is friendlier to records.

I picked RemoteLaunchCallable as an example of substantial legibility improvements from this conversion, but could try doing more as well. (OpenRewrite would be great though I am not experienced in that yet.)

Similarly, MasterToSlaveFileCallable was unnecessarily awkward to use; FilePath.Archive can be simplified this way as an example.

Testing done

None, just presuming that RemoteLaunchCallable and FilePath.Archive are well covered by existing functional tests.

Note that the version of SpotBugs we are currently running suffers from spotbugs/spotbugs#2795 which is relevant for this use case: these records are Serializable (transiently), but no serialVersionUID is meaningful since Java serialization for records is handled specially. The SpotBugs check for jenkins-core does not seem to complain for whatever reason (maybe this bug priority is already suppressed?); in my plugin doing similar work I needed to

@SuppressFBWarnings(value = "SE_NO_SERIALVERSIONID", justification = "TODO https://github.com/spotbugs/spotbugs/pull/2795 4.8.4+")

I do not feel very comfortable with the idea of reducing the typical idiom further to that using a static method as in jenkinsci/jenkins-test-harness#671 because production code might involve different Java versions (or even vendors!) between controller and agent and it is not obvious to me that this would be safe. Given

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.Serializable;
import java.nio.file.Files;
import java.nio.file.Path;
import org.junit.Test;
public class XTest {
    @Test public void x() throws Exception {
        FI fi = XTest::m;
        assertThat(fi.ap(1), is(2));
        byte[] data;
        try (var baos = new ByteArrayOutputStream(); var oos = new ObjectOutputStream(baos)) {
            oos.writeObject(fi);
            data = baos.toByteArray();
        }
        Files.write(Path.of("/tmp/fi.ser"), data);
        Object o;
        try (var bais = new ByteArrayInputStream(data); var ois = new ObjectInputStream(bais)) {
            o = ois.readObject();
        }
        fi = (FI) o;
        assertThat(fi.ap(1), is(2));
    }
    private static int m(int x) {
        return x + 1;
    }
    @FunctionalInterface interface FI extends Serializable {
        int ap(int x);
    }
}

you can see a rather complex serial form

$ strings /tmp/fi.ser 
!java.lang.invoke.SerializedLambdaoa
implMethodKind[
capturedArgst
[Ljava/lang/Object;L
capturingClasst
Ljava/lang/Class;L
functionalInterfaceClasst
Ljava/lang/String;L
functionalInterfaceMethodNameq
"functionalInterfaceMethodSignatureq
	implClassq
implMethodNameq
implMethodSignatureq
instantiatedMethodTypeq
[Ljava.lang.Object;
XTest
XTest$FIt
(I)It
XTestt

Proposed changelog entries

Developer: Introducing ControllerToAgentCallable and ControllerToAgentFileCallable with support for records in controller to agent callables

Proposed upgrade guidelines

N/A

### Submitter checklist
- [ ] The Jira issue, if it exists, is well-described.
- [ ] The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see [examples](https://github.com/jenkins-infra/jenkins.io/blob/master/content/_data/changelogs/weekly.yml)). Fill in the **Proposed upgrade guidelines** section only if there are breaking changes or changes that may require extra steps from users during upgrade.
- [ ] There is automated testing or an explanation as to why this change has no tests.
- [ ] New public classes, fields, and methods are annotated with `@Restricted` or have `@since TODO` Javadocs, as appropriate.
- [ ] New deprecations are annotated with `@Deprecated(since = "TODO")` or `@Deprecated(forRemoval = true, since = "TODO")`, if applicable.
- [ ] New or substantially changed JavaScript is not defined inline and does not call `eval` to ease future introduction of Content Security Policy (CSP) directives (see [documentation](https://www.jenkins.io/doc/developer/security/csp/)).
- [ ] For dependency updates, there are links to external changelogs and, if possible, full differentials.
- [ ] For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@Vlatombe @nevingeorgesunny @gbhat618

Before the changes are marked as ready-for-merge:

### Maintainer checklist
- [ ] There are at least two (2) approvals for the pull request and no outstanding requests for change.
- [ ] Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
- [ ] Changelog entries in the pull request title and/or **Proposed changelog entries** are accurate, human-readable, and in the imperative mood.
- [ ] Proper changelog labels are set so that the changelog can be generated automatically.
- [ ] If the change needs additional upgrade steps from users, the `upgrade-guide-needed` label is set and there is a **Proposed upgrade guidelines** section in the pull request title (see [example](https://github.com/jenkinsci/jenkins/pull/4387)).
- [ ] If it would make sense to backport the change to LTS, a Jira issue must exist, be a _Bug_ or _Improvement_, and be labeled as `lts-candidate` to be considered (see [query](https://issues.jenkins.io/issues/?filter=12146)).

jglick · 2024-10-28T16:16:22Z

core/src/main/java/hudson/Launcher.java

-        }
-
-        @Restricted(NoExternalUse.class)
-        public void setEnvVarsFilterRuleWrapper(EnvVarsFilterRuleWrapper envVarsFilterRuleWrapper) {


@daniel-beck in #4683 you added this as a setter rather than constructor parameter, perhaps out of confusion with the similarly-named field in Launcher which actually needs to be mutable?

Maybe, the original code is by Wadeck and it's been far too long at this point. If this works, the setter is restricted, so any cleanup is fine.

the setter is restricted

Not that it needed to be: the class was private to begin with.

…ntrollerToAgentCallable

… about security

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

Vlatombe · 2024-11-04T09:30:00Z

core/src/main/java/jenkins/MasterToSlaveFileCallable.java

+ * For new code, implement {@link ControllerToAgentFileCallable}
+ * which has the advantage that it can be used on {@code record}s.


Given that, should this class be deprecated to discourage any new usage, or hint existing ones to move to records?

I suppose it could be, though it feels like overkill to deprecate it merely because an aesthetically preferable alternative is now available.

Co-authored-by: Vincent Latombe <[email protected]>

timja

LGTM

/label ready-for-merge

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

Introducing ControllerToAgentCallable

faf2de3

jglick requested a review from Vlatombe October 28, 2024 15:27

jglick commented Oct 28, 2024

View reviewed changes

jglick mentioned this pull request Oct 28, 2024

[JENKINS-73835] Do not allow builds to be deleted while they are still running and ensure build discarders run after builds are fully complete #9810

Merged

jglick added 4 commits November 1, 2024 16:39

Merge branch 'master' of https://github.com/jenkinsci/jenkins into Co…

582d3e8

…ntrollerToAgentCallable

MasterToSlaveCallable can simply implement ControllerToAgentCallable

42fd2cf

jenkins.agents seems a more appropriate package; this is not really…

d7af175

… about security

ControllerToAgentFileCallable also seems helpful

9f3cd33

jglick changed the title ~~Introducing ControllerToAgentCallable~~ Introducing ControllerToAgentCallable and ControllerToAgentFileCallable Nov 1, 2024

Vlatombe reviewed Nov 4, 2024

View reviewed changes

core/src/main/java/jenkins/agents/ControllerToAgentCallable.java Outdated Show resolved Hide resolved

Vlatombe reviewed Nov 4, 2024

View reviewed changes

Vlatombe approved these changes Nov 4, 2024

View reviewed changes

jglick and others added 2 commits November 4, 2024 13:18

Stray space

f073fcb

Co-authored-by: Vincent Latombe <[email protected]>

Merge branch 'master' into ControllerToAgentCallable

bba1ff0

jglick requested a review from a team November 4, 2024 18:18

timja approved these changes Nov 4, 2024

View reviewed changes

comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Nov 4, 2024

timja added the developer Changes which impact plugin developers label Nov 4, 2024

timja merged commit abef2cc into jenkinsci:master Nov 9, 2024

jglick deleted the ControllerToAgentCallable branch November 12, 2024 19:18

jglick mentioned this pull request Nov 12, 2024

Suggesting ControllerToAgentFileCallable jenkins-infra/jenkins.io#7678

Merged

jglick mentioned this pull request Dec 6, 2024

Pick up current SpotBugs releases, kill @Nullable jenkinsci/plugin-pom#1063

Closed

jglick mentioned this pull request May 22, 2025

Recommend ControllerToAgentCallable jenkins-infra/jenkins.io#8117

Merged

This was referenced Dec 30, 2025

SlaveCommandStatistics Memory Leak with certain Docker Commands jenkinsci/support-core-plugin#942

Closed

Fix SlaveCommandStatistics memory leak with some commands jenkinsci/support-core-plugin#943

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Introducing `ControllerToAgentCallable` and `ControllerToAgentFileCallable` #9921

Introducing `ControllerToAgentCallable` and `ControllerToAgentFileCallable` #9921

Uh oh!

jglick commented Oct 28, 2024 •

edited by timja

Loading

Uh oh!

jglick Oct 28, 2024

Uh oh!

daniel-beck Oct 28, 2024

Uh oh!

jglick Oct 28, 2024

Uh oh!

Uh oh!

Vlatombe Nov 4, 2024 •

edited

Loading

Uh oh!

jglick Nov 4, 2024

Uh oh!

timja left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		* For new code, implement {@link ControllerToAgentFileCallable}
		* which has the advantage that it can be used on {@code record}s.

Uh oh!

Introducing ControllerToAgentCallable and ControllerToAgentFileCallable #9921

Introducing ControllerToAgentCallable and ControllerToAgentFileCallable #9921

Uh oh!

Conversation

jglick commented Oct 28, 2024 • edited by timja Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Desired reviewers

Uh oh!

jglick Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

daniel-beck Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

jglick Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Vlatombe Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

timja left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Introducing `ControllerToAgentCallable` and `ControllerToAgentFileCallable` #9921

Introducing `ControllerToAgentCallable` and `ControllerToAgentFileCallable` #9921

jglick commented Oct 28, 2024 •

edited by timja

Loading

Vlatombe Nov 4, 2024 •

edited

Loading