Skip to content

Commit 7c138cf

Browse files
FoxyBOAvogella
authored andcommitted
Fix progress overshoot >100% in Synchronize refresh jobs
In RefreshSubscriberParticipantJob.doRefresh and the sibling RefreshModelParticipantJob.doRefresh, the same IProgressMonitor was passed sequentially to two consumers (subscriber.refresh + SubscriberSyncInfoCollector.waitForCollector, and context.refresh + JobManager.join respectively). Each consumer treated the monitor as exclusively its own and reported its full budget against the parent, which is how the JobManager group ends up displaying 115-160 % for Synchronize jobs (reproducible with Subversive and other team providers). Wrap the parent monitor with SubMonitor.convert(monitor, 100) and split 80/20 between the two consumers, mirroring the existing 80/20 split already used in RefreshParticipantJob.initialize between setProgressGroup(group, 80) and handleProgressGroupSet(group, 20). To keep the production class shape unchanged and avoid touching SubscriberSyncInfoCollector, the SubscriberJob exposes the existing collector handoff as a small protected seam: protected void waitForCollector(IProgressMonitor monitor) { getCollector().waitForCollector(monitor); } The regression test (RefreshSubscriberParticipantJobProgressTests in org.eclipse.team.tests.core, wired into AllTeamTests) overrides this seam in a TestableJob to imitate a greedy collector. With the previous code the parent monitor records ~2000 ticks; with the fix it records ~1000, well below the 1500 threshold the test asserts. Fixes #2645 Signed-off-by: boa <foxyboa@gmail.com>
1 parent 5ccd447 commit 7c138cf

4 files changed

Lines changed: 127 additions & 4 deletions

File tree

team/bundles/org.eclipse.team.ui/src/org/eclipse/team/internal/ui/synchronize/RefreshModelParticipantJob.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import org.eclipse.core.runtime.IPath;
2323
import org.eclipse.core.runtime.IProgressMonitor;
2424
import org.eclipse.core.runtime.IStatus;
25+
import org.eclipse.core.runtime.SubMonitor;
2526
import org.eclipse.core.runtime.jobs.Job;
2627
import org.eclipse.team.core.diff.IDiff;
2728
import org.eclipse.team.core.diff.IDiffChangeEvent;
@@ -77,11 +78,12 @@ protected void doRefresh(IChangeDescription changeListener,
7778
ISynchronizationContext context = ((ModelSynchronizeParticipant)getParticipant()).getContext();
7879
try {
7980
context.getDiffTree().addDiffChangeListener((ChangeDescription)changeListener);
81+
SubMonitor sub = SubMonitor.convert(monitor, 100);
8082
// TODO: finer grained refresh
81-
context.refresh(mappings, monitor);
83+
context.refresh(mappings, sub.split(80));
8284
// Wait for any asynchronous updating to complete
8385
try {
84-
Job.getJobManager().join(context, monitor);
86+
Job.getJobManager().join(context, sub.split(20));
8587
} catch (InterruptedException e) {
8688
// Ignore
8789
}

team/bundles/org.eclipse.team.ui/src/org/eclipse/team/internal/ui/synchronize/RefreshSubscriberParticipantJob.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
import org.eclipse.core.resources.IResource;
1818
import org.eclipse.core.runtime.IProgressMonitor;
19+
import org.eclipse.core.runtime.SubMonitor;
1920
import org.eclipse.team.core.TeamException;
2021
import org.eclipse.team.core.subscribers.Subscriber;
2122
import org.eclipse.team.core.synchronize.SyncInfo;
@@ -119,11 +120,16 @@ protected void doRefresh(IChangeDescription changeListener, IProgressMonitor mon
119120
if (subscriber != null) {
120121
try {
121122
subscriber.addListener((RefreshChangeListener)changeListener);
122-
subscriber.refresh(resources, IResource.DEPTH_INFINITE, monitor);
123-
getCollector().waitForCollector(monitor);
123+
SubMonitor sub = SubMonitor.convert(monitor, 100);
124+
subscriber.refresh(resources, IResource.DEPTH_INFINITE, sub.split(80));
125+
waitForCollector(sub.split(20));
124126
} finally {
125127
subscriber.removeListener((RefreshChangeListener)changeListener);
126128
}
127129
}
128130
}
131+
132+
protected void waitForCollector(IProgressMonitor monitor) {
133+
getCollector().waitForCollector(monitor);
134+
}
129135
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
/*******************************************************************************
2+
* Copyright (c) 2026 Foxy BOA and others.
3+
*
4+
* This program and the accompanying materials
5+
* are made available under the terms of the Eclipse Public License 2.0
6+
* which accompanies this distribution, and is available at
7+
* https://www.eclipse.org/legal/epl-2.0/
8+
*
9+
* SPDX-License-Identifier: EPL-2.0
10+
*
11+
* Contributors:
12+
* Foxy BOA - initial implementation, issue #2645
13+
*******************************************************************************/
14+
package org.eclipse.team.internal.ui.synchronize;
15+
16+
import static org.junit.jupiter.api.Assertions.assertTrue;
17+
18+
import org.eclipse.core.resources.IResource;
19+
import org.eclipse.core.runtime.IProgressMonitor;
20+
import org.eclipse.core.runtime.NullProgressMonitor;
21+
import org.eclipse.core.runtime.SubMonitor;
22+
import org.eclipse.team.core.TeamException;
23+
import org.eclipse.team.core.subscribers.Subscriber;
24+
import org.eclipse.team.tests.core.mapping.ScopeTestSubscriber;
25+
import org.eclipse.team.ui.synchronize.ISynchronizePageConfiguration;
26+
import org.eclipse.team.ui.synchronize.SubscriberParticipant;
27+
import org.junit.jupiter.api.Test;
28+
29+
/**
30+
* Regression test for issue #2645: {@link RefreshSubscriberParticipantJob#doRefresh
31+
* RefreshSubscriberParticipantJob.doRefresh} must split its progress monitor between
32+
* {@code subscriber.refresh} and {@code waitForCollector} instead of handing the same
33+
* monitor to both.
34+
*/
35+
public class RefreshSubscriberParticipantJobProgressTests {
36+
37+
private static class CountingMonitor extends NullProgressMonitor {
38+
volatile double total;
39+
40+
@Override
41+
public void worked(int work) {
42+
total += work;
43+
}
44+
45+
@Override
46+
public void internalWorked(double work) {
47+
total += work;
48+
}
49+
}
50+
51+
private static class ConsumingSubscriber extends ScopeTestSubscriber {
52+
@Override
53+
public void refresh(IResource[] resources, int depth, IProgressMonitor monitor) {
54+
SubMonitor sm = SubMonitor.convert(monitor, 100);
55+
sm.worked(100);
56+
sm.done();
57+
}
58+
}
59+
60+
private static class TestableJob extends RefreshSubscriberParticipantJob {
61+
private final Subscriber subscriberOverride;
62+
63+
TestableJob(SubscriberParticipant participant, IResource[] resources, Subscriber subscriber) {
64+
super(participant, "test-job", "test-task", resources, null);
65+
this.subscriberOverride = subscriber;
66+
}
67+
68+
@Override
69+
protected Subscriber getSubscriber() {
70+
return subscriberOverride;
71+
}
72+
73+
@Override
74+
protected void waitForCollector(IProgressMonitor monitor) {
75+
SubMonitor sm = SubMonitor.convert(monitor, 100);
76+
sm.worked(100);
77+
sm.done();
78+
}
79+
80+
void invokeDoRefresh(RefreshChangeListener changeDescription, IProgressMonitor monitor) throws TeamException {
81+
doRefresh(changeDescription, monitor);
82+
}
83+
}
84+
85+
@Test
86+
public void doRefreshDoesNotOverConsumeParentMonitor() throws Exception {
87+
ConsumingSubscriber subscriber = new ConsumingSubscriber();
88+
SubscriberParticipant participant = new SubscriberParticipant() {
89+
@Override
90+
public Subscriber getSubscriber() {
91+
return subscriber;
92+
}
93+
94+
@Override
95+
protected void initializeConfiguration(ISynchronizePageConfiguration configuration) {
96+
// not needed for this test
97+
}
98+
};
99+
100+
TestableJob job = new TestableJob(participant, new IResource[0], subscriber);
101+
RefreshChangeListener changeDescription = new RefreshChangeListener(new IResource[0], null);
102+
103+
CountingMonitor parent = new CountingMonitor();
104+
parent.beginTask("test", 100);
105+
job.invokeDoRefresh(changeDescription, parent);
106+
parent.done();
107+
108+
// Without the fix, both consumers receive the same monitor and parent.total ~= 2000.
109+
// With the fix, doRefresh splits the parent 80/20 and parent.total ~= 1000.
110+
assertTrue(parent.total <= 1500,
111+
"Parent monitor over-consumed: total=" + parent.total + " (expected <= 1500)");
112+
}
113+
}

team/tests/org.eclipse.team.tests.core/src/org/eclipse/team/tests/core/AllTeamTests.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,15 @@
1313
*******************************************************************************/
1414
package org.eclipse.team.tests.core;
1515

16+
import org.eclipse.team.internal.ui.synchronize.RefreshSubscriberParticipantJobProgressTests;
1617
import org.eclipse.team.tests.core.regression.AllTeamRegressionTests;
1718
import org.junit.platform.suite.api.SelectClasses;
1819
import org.junit.platform.suite.api.Suite;
1920

2021
@Suite
2122
@SelectClasses({ //
2223
AllTeamRegressionTests.class, //
24+
RefreshSubscriberParticipantJobProgressTests.class, //
2325
RepositoryProviderTests.class, //
2426
StorageMergerTests.class, //
2527
StreamTests.class, //

0 commit comments

Comments
 (0)