Skip to content

Fix incorrect SubMonitor usage patterns #3542

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

Draft
vogella wants to merge 2 commits into
base: master
Choose a base branch
from
+20 −58
Draft

Fix incorrect SubMonitor usage patterns #3542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c845b0
Fix incorrect SubMonitor usage patterns
vogella Nov 14, 2025
c5a9b88
Version bump(s) for 4.39 stream
eclipse-platform-bot Dec 12, 2025
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
2 changes: 1 addition & 1 deletion bundles/org.eclipse.ltk.core.refactoring/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ Automatic-Module-Name: org.eclipse.ltk.core.refactoring
Bundle-ManifestVersion: 2
Bundle-Name: %pluginName
Bundle-SymbolicName: org.eclipse.ltk.core.refactoring; singleton:=true
Bundle-Version: 3.15.100.qualifier
Bundle-Version: 3.15.200.qualifier
Bundle-Activator: org.eclipse.ltk.internal.core.refactoring.RefactoringCorePlugin
Bundle-ActivationPolicy: lazy
Bundle-Vendor: %providerName
Expand Down
1 change: 0 additions & 1 deletion ...e.ltk.core.refactoring/src/org/eclipse/ltk/core/refactoring/MultiStateTextFileChange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,6 @@ public final IDocument getCurrentDocument(IProgressMonitor monitor) throws CoreE
releaseDocument(result, subMon.newChild(1));
}
}
subMon.done();
if (result == null) {
result= new Document();
}
Expand Down
26 changes: 11 additions & 15 deletions ...pse.ltk.core.refactoring/src/org/eclipse/ltk/core/refactoring/PerformChangeOperation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,22 +199,18 @@ public void setSchedulingRule(ISchedulingRule rule) {
@Override
public void run(IProgressMonitor pm) throws CoreException {
SubMonitor subMon= SubMonitor.convert(pm, 4);
try {
fChangeExecuted= false;
if (createChange()) {
// Check for cancellation before executing the change, since canceling
// during change execution is not supported
// (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=187265 ):
fCreateChangeOperation.run(subMon.split(3));
fChange= fCreateChangeOperation.getChange();
if (fChange != null) {
executeChange(subMon.newChild(1));
}
} else {
executeChange(subMon.newChild(4));
fChangeExecuted= false;
if (createChange()) {
// Check for cancellation before executing the change, since canceling
// during change execution is not supported
// (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=187265 ):
fCreateChangeOperation.run(subMon.split(3));
fChange= fCreateChangeOperation.getChange();
if (fChange != null) {
executeChange(subMon.newChild(1));
}
} finally {
subMon.done();
} else {
executeChange(subMon.newChild(4));
}
}

Expand Down
1 change: 0 additions & 1 deletion ...tk.core.refactoring/src/org/eclipse/ltk/core/refactoring/PerformRefactoringOperation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ public void run(IProgressMonitor monitor) throws CoreException {
fUndo= perform.getUndoChange();
}
} finally {
subMon.done();
if (fRefactoringContext != null) {
fRefactoringContext.dispose();
}
Expand Down
1 change: 0 additions & 1 deletion ...es/org.eclipse.ltk.core.refactoring/src/org/eclipse/ltk/core/refactoring/Refactoring.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ public RefactoringStatus checkAllConditions(IProgressMonitor pm) throws CoreExce
if (!result.hasFatalError()) {
result.merge(checkFinalConditions(subMon.split(refactoringTickProvider.getCheckFinalConditionsTicks())));
}
subMon.done();
return result;
}

Expand Down
1 change: 0 additions & 1 deletion ...les/org.eclipse.ltk.core.refactoring/src/org/eclipse/ltk/core/refactoring/TextChange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,6 @@ public Change perform(IProgressMonitor pm) throws CoreException {
throw Changes.asCoreException(e);
} finally {
releaseDocument(document, subMon.newChild(1));
subMon.done();
Copy link
Contributor

@ptziegler ptziegler Dec 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't this cause eclipse-jdt/eclipse.jdt.ui#1833 to reappear? Or is this why the call to done() in getCurrentDocument(IProgressMonitor) remains?

My understanding is that the call to done() is only irrelevant, if the progress monitor is always used correctly. But I have yet to see a project where that is actually the case.

I.e. I would expect test failures in other components, which then also need to be updated.

Copy link
Contributor Author

@vogella vogella Dec 12, 2025

Choose a reason for hiding this comment

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

Good point. So should we leave it as is?

Copy link
Contributor

@ptziegler ptziegler Dec 12, 2025

Choose a reason for hiding this comment

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

If we can already expect test failures in other components then I would definitely leave it out. For the rest, I'd have to take a closer look to see which are definitely save and which might cause side-effects.
Perhaps it makes sense to split this PR up? Have one for the removal of the isCanceled() check (which at first glance looks safe) and one for the removal of done().

}
}

Expand Down
5 changes: 0 additions & 5 deletions ...refactoring/src/org/eclipse/ltk/core/refactoring/participants/CheckConditionsContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.core.runtime.IStatus;
import org.eclipse.core.runtime.NullProgressMonitor;
import org.eclipse.core.runtime.OperationCanceledException;
import org.eclipse.core.runtime.Status;
import org.eclipse.core.runtime.SubMonitor;

Expand Down Expand Up @@ -123,11 +122,7 @@ public RefactoringStatus check(IProgressMonitor pm) throws CoreException {
SubMonitor sm= SubMonitor.convert(pm, "", values.size()); //$NON-NLS-1$
for (IConditionChecker checker : values) {
result.merge(checker.check(sm.split(1)));
if (pm.isCanceled()) {
throw new OperationCanceledException();
}
}
pm.done();
return result;
}

Expand Down
12 changes: 0 additions & 12 deletions ...actoring/src/org/eclipse/ltk/core/refactoring/participants/ProcessorBasedRefactoring.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,9 +232,6 @@ public RefactoringStatus checkFinalConditions(IProgressMonitor pm) throws CoreEx
if (result.hasFatalError()) {
return result;
}
if (sm.isCanceled()) {
throw new OperationCanceledException();
}

SharableParticipants sharableParticipants= new SharableParticipants(); // must not be shared when checkFinalConditions is called again
RefactoringParticipant[] loadedParticipants= getProcessor().loadParticipants(result, sharableParticipants);
Expand Down Expand Up @@ -270,9 +267,6 @@ public RefactoringStatus checkFinalConditions(IProgressMonitor pm) throws CoreEx

stats.endRun();

if (sm2.isCanceled()) {
throw new OperationCanceledException();
}
}
if (result.hasFatalError()) {
return result;
Expand All @@ -292,9 +286,6 @@ public Change createChange(IProgressMonitor pm) throws CoreException {
try {
SubMonitor sm= SubMonitor.convert(pm, RefactoringCoreMessages.ProcessorBasedRefactoring_create_change, fParticipants.size() * 2 + 1);
Change processorChange= getProcessor().createChange(sm.split(1));
if (sm.isCanceled()) {
throw new OperationCanceledException();
}

fTextChangeMap= new HashMap<>();
addToTextChangeMap(processorChange);
Expand Down Expand Up @@ -334,9 +325,6 @@ public Change createChange(IProgressMonitor pm) throws CoreException {
disableParticipant(participant, e);
throw e;
}
if (sm.isCanceled()) {
throw new OperationCanceledException();
}
}

fTextChangeMap= null;
Expand Down
2 changes: 1 addition & 1 deletion bundles/org.eclipse.ui.ide/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Bundle-Name: %Plugin.name
Bundle-SymbolicName: org.eclipse.ui.ide; singleton:=true
Bundle-Version: 3.22.800.qualifier
Bundle-Version: 3.22.900.qualifier
Bundle-Activator: org.eclipse.ui.internal.ide.IDEWorkbenchPlugin
Bundle-ActivationPolicy: lazy
Bundle-Vendor: %Plugin.providerName
Expand Down
2 changes: 0 additions & 2 deletions ...s/org.eclipse.ui.ide/src/org/eclipse/ui/internal/wizards/datatransfer/SmartImportJob.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,6 @@ private Set<IProject> searchAndImportChildrenProjectsRecursively(final IContaine
for (CrawlFolderJob job : jobs) {
job.join(0, subMonitor.split(1));
}
subMonitor.done();
return res;
}

Expand Down Expand Up @@ -512,7 +511,6 @@ private Set<IProject> importProjectAndChildrenRecursively(final IContainer conta
// make sure this folder isn't going to be processed again
excludedPaths.add(project.getLocation());
}
subMonitor.done();
return projectFromCurrentContainer;
}

Expand Down
21 changes: 7 additions & 14 deletions bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/SaveableHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,21 +184,14 @@ private static boolean saveModels(ISaveablesSource modelSource, final IWorkbench
IRunnableWithProgress progressOp = monitor -> {
IProgressMonitor monitorWrap = new EventLoopProgressMonitor(monitor);
SubMonitor subMonitor = SubMonitor.convert(monitorWrap, WorkbenchMessages.Save, dirtyModels.size());
try {
for (Saveable model : dirtyModels) {
// handle case where this model got saved as a result of
// saving another
if (!model.isDirty()) {
subMonitor.worked(1);
continue;
}
doSaveModel(model, subMonitor.split(1), window, confirm);
if (subMonitor.isCanceled()) {
break;
}
for (Saveable model : dirtyModels) {
// handle case where this model got saved as a result of
// saving another
if (!model.isDirty()) {
subMonitor.worked(1);
continue;
}
} finally {
monitorWrap.done();
doSaveModel(model, subMonitor.split(1), window, confirm);
}
};

Expand Down
4 changes: 0 additions & 4 deletions bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/SaveablesList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -796,11 +796,7 @@ public boolean saveModels(final List<Saveable> finalModels, final IShellProvider
continue;
}
SaveableHelper.doSaveModel(model, subMonitor.split(1), shellProvider, blockUntilSaved);
if (subMonitor.isCanceled()) {
break;
}
}
monitorWrap.done();
};

// Do the save.
Expand Down
Loading