[JENKINS-26380] Terminate nodes correctly #2
Conversation
|
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
| if (terminating) { | ||
| return; | ||
| } | ||
| terminating = true; |
There was a problem hiding this comment.
Seems like an AtomicBoolean would be more idiomatic, but perhaps you need the monitor on OnceRetentionStrategy.this anyway to be exclusive with the threadPoolForRemoting thread?
There was a problem hiding this comment.
correct we want the lock here to ensure exclusivity of starting the background task
|
Do not really follow the implications of the change, but seems OK to merge to me. |
|
Is this intended to be a final fix of JENKINS-26380, or do you expect other retention strategies are also in danger from the core locking change? |
|
@jglick other retention strategies are already broken if they didn't acquire the lock on Queue before checking idle and then in the background repeat after reacquiring the lock on Queue. The core change makes it easier to do that. It does not make sense to add the lock to Jenkins.removeNode as there may be valid cases where you want to remove the node and jobs in progress be damned... hence it is up to the caller to ensure they have the correct lock based on whether the node being idle is important to them or not |
[JENKINS-26380] Terminate nodes correctly
JENKINS-26380
Supersedes #1.