Skip to content

Commit 01eda3a

Browse files
ljacometljacomet
committed
Address review feedback on worker-lease retry changes
- Document why ExponentialBackoff.backoffPeriodFor adds 1 to the random slot count: without it, iteration 1 produces a 0ms backoff (nextInt(1) is always 0), turning the first retry into a tight loop. Standard exponential-backoff guidance is to always sleep at least one slot, so the change applies cleanly to all callers. - Make TestWorkerLeaseService.tryRunAsWorkerThread use Optional.of so the test fixture matches the production contract (null returns are not allowed and must throw). - Rename shouldExit to shouldExitWithExtraWorkerInvalidation and add Javadoc, since the method has the side effect of invalidating the worker token in the extra-worker case. - Rename workerAboutToBlockForLease to workerStartedRetrying in DefaultBuildOperationQueueTest to reflect the new retry-based flow.
1 parent d51476f commit 01eda3a

4 files changed

Lines changed: 17 additions & 9 deletions

File tree

‎platforms/core-runtime/time/src/main/java/org/gradle/internal/time/ExponentialBackoff.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,9 @@ public <T> T retryUntil(Query<T> query) throws IOException, InterruptedException
7979
}
8080

8181
long backoffPeriodFor(int iteration) {
82+
// The +1 ensures every retry waits at least one slot. Without it, iteration 1 would
83+
// produce a 0ms backoff (since nextInt(1) is always 0), turning the first retry into a
84+
// tight loop. Standard exponential-backoff guidance is to always sleep at least one slot.
8285
return (random.nextInt(Math.min(iteration, CAP_FACTOR)) + 1) * slotTime;
8386
}
8487

‎subprojects/core/src/main/java/org/gradle/internal/operations/DefaultBuildOperationQueue.java‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ private boolean waitForNextOperation() {
261261
lock.lock();
262262
try {
263263
while (true) {
264-
if (shouldExit()) {
264+
if (shouldExitWithExtraWorkerInvalidation()) {
265265
return false;
266266
}
267267
if (!helper.isQueueEmpty()) {
@@ -278,8 +278,13 @@ private boolean waitForNextOperation() {
278278
}
279279
}
280280

281+
/**
282+
* Returns {@code true} if this worker should exit, and as a side effect invalidates the
283+
* worker token in the extra-worker case so that the worker count drops immediately rather
284+
* than waiting for {@link #runOperations()} to complete.
285+
*/
281286
@GuardedBy("lock")
282-
private boolean shouldExit() {
287+
private boolean shouldExitWithExtraWorkerInvalidation() {
283288
if (token != null && !token.isValid()) {
284289
return true;
285290
}
@@ -311,15 +316,15 @@ private void runBatch() {
311316

312317
private int runBatchWithLeaseRetry() {
313318
try {
314-
// Retry acquiring the lease forever, or until we `shouldExit()`.
319+
// Retry acquiring the lease forever, or until we `shouldExitWithExtraWorkerInvalidation()`.
315320
return ExponentialBackoff.of(Integer.MAX_VALUE, TimeUnit.MILLISECONDS).retryUntil(() -> {
316321
Optional<Integer> result = workerLeases.tryRunAsWorkerThread(this::executePendingWork);
317322
if (result.isPresent()) {
318323
return ExponentialBackoff.Result.successful(result.get());
319324
}
320325
lock.lock();
321326
try {
322-
if (shouldExit()) {
327+
if (shouldExitWithExtraWorkerInvalidation()) {
323328
return ExponentialBackoff.Result.successful(0);
324329
}
325330
} finally {

‎subprojects/core/src/test/groovy/org/gradle/internal/operations/DefaultBuildOperationQueueTest.groovy‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,7 @@ class DefaultBuildOperationQueueTest extends Specification {
284284
given:
285285
// Slightly modified from setupQueue to allow certain injection points.
286286
def mainThread = Thread.currentThread()
287-
def workerAboutToBlockForLease = new CountDownLatch(1)
287+
def workerStartedRetrying = new CountDownLatch(1)
288288
def executedByMain = new AtomicInteger()
289289
def executedByOther = new AtomicInteger()
290290
def recordingWorker = { TestBuildOperation op ->
@@ -302,7 +302,7 @@ class DefaultBuildOperationQueueTest extends Specification {
302302
<T> Optional<T> tryRunAsWorkerThread(Factory<T> action) {
303303
// Called repeatedly from the retry loop; CountDownLatch tolerates extra countDown() calls.
304304
if (Thread.currentThread() !== mainThread) {
305-
workerAboutToBlockForLease.countDown()
305+
workerStartedRetrying.countDown()
306306
}
307307
return super.tryRunAsWorkerThread(action)
308308
}
@@ -323,7 +323,7 @@ class DefaultBuildOperationQueueTest extends Specification {
323323
and:
324324
// Wait until the worker has tried and failed to acquire a lease.
325325
// This ensures that the main thread will be needed to progress the queue.
326-
assert workerAboutToBlockForLease.await(10, TimeUnit.SECONDS)
326+
assert workerStartedRetrying.await(10, TimeUnit.SECONDS)
327327

328328
and:
329329
operationQueue.waitForCompletion()
@@ -334,7 +334,7 @@ class DefaultBuildOperationQueueTest extends Specification {
334334
}
335335

336336
@Timeout(value = 30, unit = TimeUnit.SECONDS)
337-
def "starved worker exits via shouldExit when the queue is cancelled"() {
337+
def "starved worker exits via shouldExitWithExtraWorkerInvalidation when the queue is cancelled"() {
338338
given:
339339
def mainThread = Thread.currentThread()
340340
def workerStartedRetrying = new CountDownLatch(1)

‎testing/internal-testing/src/main/groovy/org/gradle/test/fixtures/work/TestWorkerLeaseService.groovy‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ class TestWorkerLeaseService implements WorkerLeaseService {
9292

9393
@Override
9494
<T> Optional<T> tryRunAsWorkerThread(Factory<T> action) {
95-
return Optional.ofNullable(action.create())
95+
return Optional.of(action.create())
9696
}
9797

9898
@Override

0 commit comments

Comments
 (0)