-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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.
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
Temporary memory leak in FutureImpl.executors
#8640
Conversation
j.buildAndAssertSuccess(p); | ||
Computer computer = agent.toComputer(); | ||
WeakReference<Computer> computerRef = new WeakReference<>(computer); | ||
WeakReference<Channel> channelRef = new WeakReference<>((Channel) computer.getChannel()); |
There was a problem hiding this comment.
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.
…into computersCollected
@@ -95,4 +95,8 @@ public synchronized void setAsCancelled() { | |||
synchronized void addExecutor(@NonNull Executor executor) { | |||
this.executors.add(executor); | |||
} | |||
|
|||
synchronized void finished() { | |||
executors.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/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!
Alarmed by this thread I tried to see if I could reproduce a memory leak from running K8s builds with WebSocket. I could not reproduce any major leak, but did find some minor issues.
Testing done
Without patch, the test fails with this reference chain:
LeftItem
s are kept around for 5m.Proposed changelog entries
Before the changes are marked as
ready-for-merge
:Maintainer checklist