Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Temporary memory leak in FutureImpl.executors #8640

Merged
merged 5 commits into from
Nov 8, 2023
Merged

Temporary memory leak in FutureImpl.executors #8640

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7854e16
Temporary memory leak in `FutureImpl.executors`
jglick Oct 25, 2023
99567eb
https://github.com/jenkinsci/jenkins-test-harness/pull/669 released
jglick Oct 25, 2023
4b62cc2
Merge branch 'master' of https://github.com/jenkinsci/jenkins into co…
jglick Oct 27, 2023
d564e29
Skip new test on Windows https://github.com/jenkinsci/jenkins/pull/86…
jglick Oct 27, 2023
23e873a
Merge branch 'computersCollected' of https://github.com/jglick/jenkin…
jglick Oct 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions core/src/main/java/hudson/model/queue/FutureImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,8 @@ public synchronized void setAsCancelled() {
synchronized void addExecutor(@NonNull Executor executor) {
this.executors.add(executor);
}

synchronized void finished() {
executors.clear();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jenkinsci/jenkins-test-harness#643 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes jenkinsci/jenkins-test-harness#672 is still on my list of things to look at, but I believe it is only tangentially related to this PR in that they both happen to involve code run from WorkUnitContext.synchronizeEnd; this PR is about the executors list, which is used only if Future.cancel is called, and does not interact directly with the Future done condition.

}
}
1 change: 1 addition & 0 deletions core/src/main/java/hudson/model/queue/WorkUnitContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ public void synchronizeEnd(Executor e, Queue.Executable executable, Throwable pr
}
}
}
future.finished();
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion test/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ THE SOFTWARE.
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>jenkins-test-harness</artifactId>
<version>2090.v6d1706e434a_b_</version>
<!-- TODO https://github.com/jenkinsci/jenkins-test-harness/pull/669 -->
jglick marked this conversation as resolved.
Show resolved Hide resolved
<version>2095.v04e536e1c3d2</version>
<scope>test</scope>
<exclusions>
<exclusion>
Expand Down
20 changes: 20 additions & 0 deletions test/src/test/java/hudson/model/ComputerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,11 @@

import hudson.ExtensionList;
import hudson.diagnosis.OldDataMonitor;
import hudson.remoting.Channel;
import hudson.slaves.DumbSlave;
import hudson.slaves.OfflineCause;
import java.io.File;
import java.lang.ref.WeakReference;
import java.net.HttpURLConnection;
import java.nio.charset.StandardCharsets;
import java.util.Map;
Expand All @@ -66,6 +68,7 @@
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.JenkinsRule.WebClient;
import org.jvnet.hudson.test.LoggerRule;
import org.jvnet.hudson.test.MemoryAssert;
import org.jvnet.hudson.test.MockAuthorizationStrategy;
import org.jvnet.hudson.test.MockFolder;
import org.jvnet.hudson.test.SmokeTest;
Expand Down Expand Up @@ -263,4 +266,21 @@ public void testTerminatedNodeAjaxExecutorsDoesNotShowTrace() throws Exception {
j.assertBuildStatus(Result.FAILURE, j.waitForCompletion(b));
}

@Test
public void computersCollected() throws Exception {
jglick marked this conversation as resolved.
Show resolved Hide resolved
DumbSlave agent = j.createOnlineSlave();
FreeStyleProject p = j.createFreeStyleProject();
p.setAssignedNode(agent);
j.buildAndAssertSuccess(p);
Computer computer = agent.toComputer();
WeakReference<Computer> computerRef = new WeakReference<>(computer);
WeakReference<Channel> channelRef = new WeakReference<>((Channel) computer.getChannel());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case there was some problem with the Channel separately from the Computer. I could not reproduce any in this test, other than jenkinsci/remoting#694 which was reproduced interactively.

computer.disconnect(null);
computer = null;
j.jenkins.removeNode(agent);
agent = null;
MemoryAssert.assertGC(computerRef, false);
MemoryAssert.assertGC(channelRef, false);
}

}