Skip to content

Use AtomicInteger in JobGroupTest.testShouldCancel_4 #2668

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

Open
trancexpress wants to merge 1 commit into
base: master
Choose a base branch
from
+6 −8
Open

Use AtomicInteger in JobGroupTest.testShouldCancel_4 #2668

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 6 additions & 8 deletions .../org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobGroupTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicIntegerArray;
import java.util.concurrent.atomic.AtomicLong;
import org.eclipse.core.runtime.IProgressMonitor;
Expand Down Expand Up @@ -1246,16 +1247,12 @@ public IStatus run(IProgressMonitor monitor) {
public void testShouldCancel_4() {
final int NUM_JOBS = 1000;
final int NUM_JOBS_LIMIT = 100;
final int numShouldCancelCalled[] = {0};
final AtomicInteger numShouldCancelCalled = new AtomicInteger(0);
final TestBarrier2 barrier = new TestBarrier2();
final JobGroup jobGroup = new JobGroup("JobGroup", 10, NUM_JOBS) {
@Override
protected boolean shouldCancel(IStatus lastCompletedJobResult, int numberOfFailedJobs, int numberOfCanceledJobs) {
numShouldCancelCalled[0]++;
if (numShouldCancelCalled[0] == NUM_JOBS_LIMIT) {
return true;
}
return false;
return numShouldCancelCalled.incrementAndGet() >= NUM_JOBS_LIMIT;
Copy link
Copy Markdown
Member

@fedejeanne fedejeanne May 22, 2026

Choose a reason for hiding this comment

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

Nice catch, checking for == was wrong because of the race condition (2 threads executing numShouldCancelCalled[0]++; before one of them reached the if below would skip numbers).

FTR it is now safe to simply do:

return numShouldCancelCalled.incrementAndGet() == NUM_JOBS_LIMIT;

... because the comparison (==) uses the now stable value that comes from incrementAndGet() (no race condition there).

}
};
for (int i = 0; i < NUM_JOBS; i++) {
Expand All @@ -1274,10 +1271,11 @@ public IStatus run(IProgressMonitor monitor) {
// Allow the jobs to proceed to run.
barrier.setStatus(TestBarrier2.STATUS_START);
waitForCompletion(jobGroup);
assertTrue(numShouldCancelCalled[0] >= NUM_JOBS_LIMIT);
int called = numShouldCancelCalled.get();
assertTrue(called >= NUM_JOBS_LIMIT);
Copy link
Copy Markdown
Member

@fedejeanne fedejeanne May 22, 2026

Choose a reason for hiding this comment

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

Suggested change
assertTrue(called >= NUM_JOBS_LIMIT);
assertThat(called).isGreaterThanOrEqualTo(NUM_JOBS_LIMIT);

// Verify that the group is canceled in a reasonable time,
// i.e only 10 jobs are allowed to run after the shouldCancel method returned true.
assertTrue(numShouldCancelCalled[0] < NUM_JOBS_LIMIT + 10);
assertTrue(called < NUM_JOBS_LIMIT + 10, "Expected fewer jobs to run but got: " + called);
Copy link
Copy Markdown
Member

@fedejeanne fedejeanne May 22, 2026

Choose a reason for hiding this comment

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

Suggested change
assertTrue(called < NUM_JOBS_LIMIT + 10, "Expected fewer jobs to run but got: " + called);
assertThat(called).isLessThan(NUM_JOBS_LIMIT + 10);

}

/**
Expand Down
Loading