Skip to content

Commit b39c458

Browse files
vogellavogella
committed
Fix incorrect SubMonitor usage patterns
This commit addresses several SubMonitor anti-patterns identified throughout the codebase, based on best practices from the Eclipse article on Progress Monitors. Changes made: 1. Remove unnecessary null checks before SubMonitor.convert() - SubMonitor.convert() can handle null monitors - Fixed in team Policy.java files (core and ui) 2. Remove unnecessary SubMonitor.done() calls - SubMonitor.done() is a no-op and should not be called - Removed 27 instances across resources and team bundles: * org.eclipse.core.filesystem: LocalFile.java (2 calls) * org.eclipse.core.resources: File.java (6), Folder.java (1), Project.java (3), SaveManager.java (4), Workspace.java (7) * org.eclipse.compare.core: RangeComparatorLCS.java (1) * org.eclipse.compare: AddFromHistoryAction.java (1) * org.eclipse.team.core: BundleImporterDelegate.java (1), Subscriber.java (1) 3. Remove redundant cancellation checks before split() - SubMonitor.split() already checks for cancellation - Fixed in AutoBuildJob.java Reference: https://www.eclipse.org/articles/Article-Progress-Monitors/article.html
1 parent 3e8e40e commit b39c458

File tree

13 files changed

+244
-309
lines changed

13 files changed

+244
-309
lines changed

‎resources/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/local/LocalFile.java‎

Lines changed: 41 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -179,8 +179,6 @@ protected void copyFile(IFileInfo sourceInfo, IFileStore destination, int option
179179
Policy.error(EFS.ERROR_EXISTS, NLS.bind(Messages.fileExists, target.filePath), e);
180180
} catch (IOException e) {
181181
Policy.error(EFS.ERROR_WRITE, NLS.bind(Messages.failedCopy, this.filePath, target.filePath), e);
182-
} finally {
183-
subMonitor.done();
184182
}
185183
} else {
186184
super.copyFile(sourceInfo, destination, options, monitor);
@@ -457,55 +455,51 @@ public void move(IFileStore destFile, int options, IProgressMonitor monitor) thr
457455
File destination = destinationFile.file;
458456
boolean overwrite = (options & EFS.OVERWRITE) != 0;
459457
SubMonitor subMonitor = SubMonitor.convert(monitor, NLS.bind(Messages.moving, source.getAbsolutePath()), 1);
458+
//this flag captures case renaming on a case-insensitive OS, or moving
459+
//two equivalent files in an environment that supports symbolic links.
460+
//in these cases we NEVER want to delete anything
461+
boolean sourceEqualsDest = false;
460462
try {
461-
//this flag captures case renaming on a case-insensitive OS, or moving
462-
//two equivalent files in an environment that supports symbolic links.
463-
//in these cases we NEVER want to delete anything
464-
boolean sourceEqualsDest = false;
465-
try {
466-
sourceEqualsDest = isSameFile(source, destination);
467-
} catch (IOException e) {
468-
String message = NLS.bind(Messages.couldNotMove, source.getAbsolutePath());
469-
Policy.error(EFS.ERROR_WRITE, message, e);
470-
}
471-
if (!sourceEqualsDest && !overwrite && destination.exists()) {
472-
String message = NLS.bind(Messages.fileExists, destination.getAbsolutePath());
473-
Policy.error(EFS.ERROR_EXISTS, message);
474-
}
475-
if (source.renameTo(destination)) {
476-
// double-check to ensure we really did move
477-
// since java.io.File#renameTo sometimes lies
478-
if (!sourceEqualsDest && source.exists()) {
479-
// XXX: document when this occurs
480-
if (destination.exists()) {
481-
// couldn't delete the source so remove the destination and throw an error
482-
// XXX: if we fail deleting the destination, the destination (root) may still exist
483-
new LocalFile(destination).delete(EFS.NONE, null);
484-
String message = NLS.bind(Messages.couldnotDelete, source.getAbsolutePath());
485-
Policy.error(EFS.ERROR_DELETE, message);
486-
}
487-
// source exists but destination doesn't so try to copy below
488-
} else {
489-
// destination.exists() returns false for broken links, this has to be handled explicitly
490-
if (!destination.exists() && !destFile.fetchInfo().getAttribute(EFS.ATTRIBUTE_SYMLINK)) {
491-
// neither the source nor the destination exist. this is REALLY bad
492-
String message = NLS.bind(Messages.failedMove, source.getAbsolutePath(), destination.getAbsolutePath());
493-
Policy.error(EFS.ERROR_WRITE, message);
494-
}
495-
// the move was successful
496-
return;
463+
sourceEqualsDest = isSameFile(source, destination);
464+
} catch (IOException e) {
465+
String message = NLS.bind(Messages.couldNotMove, source.getAbsolutePath());
466+
Policy.error(EFS.ERROR_WRITE, message, e);
467+
}
468+
if (!sourceEqualsDest && !overwrite && destination.exists()) {
469+
String message = NLS.bind(Messages.fileExists, destination.getAbsolutePath());
470+
Policy.error(EFS.ERROR_EXISTS, message);
471+
}
472+
if (source.renameTo(destination)) {
473+
// double-check to ensure we really did move
474+
// since java.io.File#renameTo sometimes lies
475+
if (!sourceEqualsDest && source.exists()) {
476+
// XXX: document when this occurs
477+
if (destination.exists()) {
478+
// couldn't delete the source so remove the destination and throw an error
479+
// XXX: if we fail deleting the destination, the destination (root) may still exist
480+
new LocalFile(destination).delete(EFS.NONE, null);
481+
String message = NLS.bind(Messages.couldnotDelete, source.getAbsolutePath());
482+
Policy.error(EFS.ERROR_DELETE, message);
497483
}
484+
// source exists but destination doesn't so try to copy below
485+
} else {
486+
// destination.exists() returns false for broken links, this has to be handled explicitly
487+
if (!destination.exists() && !destFile.fetchInfo().getAttribute(EFS.ATTRIBUTE_SYMLINK)) {
488+
// neither the source nor the destination exist. this is REALLY bad
489+
String message = NLS.bind(Messages.failedMove, source.getAbsolutePath(), destination.getAbsolutePath());
490+
Policy.error(EFS.ERROR_WRITE, message);
491+
}
492+
// the move was successful
493+
return;
498494
}
499-
// for some reason renameTo didn't work
500-
if (sourceEqualsDest) {
501-
String message = NLS.bind(Messages.couldNotMove, source.getAbsolutePath());
502-
Policy.error(EFS.ERROR_WRITE, message, null);
503-
}
504-
// fall back to default implementation
505-
super.move(destFile, options, subMonitor.newChild(1));
506-
} finally {
507-
subMonitor.done();
508495
}
496+
// for some reason renameTo didn't work
497+
if (sourceEqualsDest) {
498+
String message = NLS.bind(Messages.couldNotMove, source.getAbsolutePath());
499+
Policy.error(EFS.ERROR_WRITE, message, null);
500+
}
501+
// fall back to default implementation
502+
super.move(destFile, options, subMonitor.newChild(1));
509503
}
510504

511505
private boolean isSameFile(File source, File destination) throws IOException {

‎resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/events/AutoBuildJob.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ public void preferenceChange(PreferenceChangeEvent event) {
297297
public IStatus run(IProgressMonitor monitor) {
298298
SubMonitor subMonitor = SubMonitor.convert(monitor, 1);
299299
synchronized (this) {
300-
if (subMonitor.isCanceled() || isInterrupted()) {
300+
if (isInterrupted()) {
301301
return canceled();
302302
}
303303
}

‎resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/File.java‎

Lines changed: 42 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,6 @@ public void appendContents(InputStream content, int updateFlags, IProgressMonito
9191
}
9292
} catch (IOException streamCloseIgnored) {
9393
// ignore;
94-
} finally {
95-
subMonitor.done();
9694
}
9795
}
9896

@@ -163,8 +161,6 @@ public void create(InputStream content, int updateFlags, IProgressMonitor monito
163161
}
164162
} catch (IOException streamCloseIgnored) {
165163
// ignore;
166-
} finally {
167-
subMonitor.done();
168164
}
169165
}
170166

@@ -177,38 +173,34 @@ public void create(InputStream content, boolean force, IProgressMonitor monitor)
177173
@Override
178174
public void create(byte[] content, int updateFlags, IProgressMonitor monitor) throws CoreException {
179175
SubMonitor subMonitor = SubMonitor.convert(monitor, NLS.bind(Messages.resources_creating, getFullPath()), 100);
176+
checkValidPath(path, FILE, true);
177+
final ISchedulingRule rule = workspace.getRuleFactory().createRule(this);
180178
try {
181-
checkValidPath(path, FILE, true);
182-
final ISchedulingRule rule = workspace.getRuleFactory().createRule(this);
183-
try {
184-
workspace.prepareOperation(rule, subMonitor.newChild(1));
185-
checkCreatable();
186-
workspace.beginOperation(true);
187-
IFileStore store = getStore();
188-
IFileInfo localInfo = create(updateFlags, subMonitor.newChild(40), store);
189-
boolean local = content != null;
190-
if (local) {
191-
try {
192-
internalSetContents(content, localInfo, updateFlags, false, subMonitor.newChild(59));
193-
} catch (CoreException | OperationCanceledException e) {
194-
// CoreException when a problem happened creating the file on disk
195-
// OperationCanceledException when the operation of setting contents has been
196-
// canceled
197-
// In either case delete from the workspace and disk
198-
workspace.deleteResource(this);
199-
store.delete(EFS.NONE, null);
200-
throw e;
201-
}
179+
workspace.prepareOperation(rule, subMonitor.newChild(1));
180+
checkCreatable();
181+
workspace.beginOperation(true);
182+
IFileStore store = getStore();
183+
IFileInfo localInfo = create(updateFlags, subMonitor.newChild(40), store);
184+
boolean local = content != null;
185+
if (local) {
186+
try {
187+
internalSetContents(content, localInfo, updateFlags, false, subMonitor.newChild(59));
188+
} catch (CoreException | OperationCanceledException e) {
189+
// CoreException when a problem happened creating the file on disk
190+
// OperationCanceledException when the operation of setting contents has been
191+
// canceled
192+
// In either case delete from the workspace and disk
193+
workspace.deleteResource(this);
194+
store.delete(EFS.NONE, null);
195+
throw e;
202196
}
203-
setLocal(local);
204-
} catch (OperationCanceledException e) {
205-
workspace.getWorkManager().operationCanceled();
206-
throw e;
207-
} finally {
208-
workspace.endOperation(rule, true);
209197
}
198+
setLocal(local);
199+
} catch (OperationCanceledException e) {
200+
workspace.getWorkManager().operationCanceled();
201+
throw e;
210202
} finally {
211-
subMonitor.done();
203+
workspace.endOperation(rule, true);
212204
}
213205
}
214206

@@ -244,7 +236,6 @@ IFileInfo create(int updateFlags, IProgressMonitor subMonitor, IFileStore store)
244236
throw new ResourceException(IResourceStatus.FAILED_WRITE_LOCAL, getFullPath(), message, null);
245237
}
246238
}
247-
subMonitor.done();
248239

249240
workspace.createResource(this, updateFlags);
250241
return localInfo;
@@ -510,39 +501,33 @@ public void setContents(InputStream content, int updateFlags, IProgressMonitor m
510501
}
511502
} catch (IOException streamCloseIgnored) {
512503
// ignore;
513-
} finally {
514-
subMonitor.done();
515504
}
516505
}
517506
@Override
518507
public void setContents(byte[] content, int updateFlags, IProgressMonitor monitor) throws CoreException {
519508
String message = NLS.bind(Messages.resources_settingContents, getFullPath());
520509
SubMonitor subMonitor = SubMonitor.convert(monitor, message, 100);
510+
if (workspace.shouldValidate) {
511+
workspace.validateSave(this);
512+
}
513+
final ISchedulingRule rule = workspace.getRuleFactory().modifyRule(this);
514+
SubMonitor newChild = subMonitor.newChild(1);
521515
try {
522-
if (workspace.shouldValidate) {
523-
workspace.validateSave(this);
524-
}
525-
final ISchedulingRule rule = workspace.getRuleFactory().modifyRule(this);
526-
SubMonitor newChild = subMonitor.newChild(1);
527-
try {
528-
workspace.prepareOperation(rule, newChild);
529-
ResourceInfo info = getResourceInfo(false, false);
530-
checkAccessible(getFlags(info));
531-
workspace.beginOperation(true);
532-
IFileInfo fileInfo = getStore().fetchInfo();
533-
if (BitMask.isSet(updateFlags, IResource.DERIVED)) {
534-
// update of derived flag during IFile.write:
535-
info.set(ICoreConstants.M_DERIVED);
536-
}
537-
internalSetContents(content, fileInfo, updateFlags, false, subMonitor.newChild(99));
538-
} catch (OperationCanceledException e) {
539-
workspace.getWorkManager().operationCanceled();
540-
throw e;
541-
} finally {
542-
workspace.endOperation(rule, true);
516+
workspace.prepareOperation(rule, newChild);
517+
ResourceInfo info = getResourceInfo(false, false);
518+
checkAccessible(getFlags(info));
519+
workspace.beginOperation(true);
520+
IFileInfo fileInfo = getStore().fetchInfo();
521+
if (BitMask.isSet(updateFlags, IResource.DERIVED)) {
522+
// update of derived flag during IFile.write:
523+
info.set(ICoreConstants.M_DERIVED);
543524
}
525+
internalSetContents(content, fileInfo, updateFlags, false, subMonitor.newChild(99));
526+
} catch (OperationCanceledException e) {
527+
workspace.getWorkManager().operationCanceled();
528+
throw e;
544529
} finally {
545-
subMonitor.done();
530+
workspace.endOperation(rule, true);
546531
}
547532
}
548533

@@ -616,7 +601,6 @@ public void setCharset(String newCharset, IProgressMonitor monitor) throws CoreE
616601
workspace.getWorkManager().operationCanceled();
617602
throw e;
618603
} finally {
619-
subMonitor.done();
620604
workspace.endOperation(rule, true);
621605
}
622606
}

‎resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Folder.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,6 @@ public void create(int updateFlags, boolean local, IProgressMonitor monitor) thr
118118
workspace.getWorkManager().operationCanceled();
119119
throw e;
120120
} finally {
121-
subMonitor.done();
122121
workspace.endOperation(rule, true);
123122
}
124123
}

‎resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Project.java‎

Lines changed: 27 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,6 @@ public void close(IProgressMonitor monitor) throws CoreException {
252252
workspace.getWorkManager().operationCanceled();
253253
throw e;
254254
} finally {
255-
subMonitor.done();
256255
workspace.endOperation(rule, true);
257256
}
258257
}
@@ -360,7 +359,6 @@ public void create(IProjectDescription description, int updateFlags, IProgressMo
360359
workspace.getWorkManager().operationCanceled();
361360
throw e;
362361
} finally {
363-
subMonitor.done();
364362
workspace.endOperation(rule, true);
365363
}
366364
}
@@ -612,43 +610,39 @@ public void run(IProgressMonitor innerMonitor) throws CoreException {
612610
final ISchedulingRule notificationsRule = relaxed ? null : workspace.getRoot();
613611
SubMonitor subMonitor = SubMonitor.convert(innerMonitor, 100);
614612
try {
615-
try {
616-
workspace.prepareOperation(notificationsRule, innerMonitor);
617-
if (!shouldBuild()) {
618-
return;
619-
}
620-
workspace.beginOperation(true);
621-
workspace.aboutToBuild(Project.this, trigger);
622-
} finally {
623-
workspace.endOperation(notificationsRule, false);
613+
workspace.prepareOperation(notificationsRule, innerMonitor);
614+
if (!shouldBuild()) {
615+
return;
624616
}
617+
workspace.beginOperation(true);
618+
workspace.aboutToBuild(Project.this, trigger);
619+
} finally {
620+
workspace.endOperation(notificationsRule, false);
621+
}
622+
try {
623+
IStatus result;
624+
workspace.prepareOperation(projectBuildRule, innerMonitor);
625+
// don't open the tree eagerly because it will be wasted if no build occurs
626+
workspace.beginOperation(false);
627+
result = workspace.getBuildManager().build(config, trigger, builderName, args,
628+
subMonitor.split(100));
629+
if (!result.isOK()) {
630+
throw new ResourceException(result);
631+
}
632+
} finally {
633+
workspace.endOperation(projectBuildRule, false);
625634
try {
626-
IStatus result;
627-
workspace.prepareOperation(projectBuildRule, innerMonitor);
628-
// don't open the tree eagerly because it will be wasted if no build occurs
635+
workspace.prepareOperation(notificationsRule, innerMonitor);
636+
// don't open the tree eagerly because it will be wasted if no change occurs
629637
workspace.beginOperation(false);
630-
result = workspace.getBuildManager().build(config, trigger, builderName, args,
631-
subMonitor.split(100));
632-
if (!result.isOK()) {
633-
throw new ResourceException(result);
638+
workspace.broadcastBuildEvent(Project.this, IResourceChangeEvent.POST_BUILD, trigger);
639+
// building may close the tree, so open it
640+
if (workspace.getElementTree().isImmutable()) {
641+
workspace.newWorkingTree();
634642
}
635643
} finally {
636-
workspace.endOperation(projectBuildRule, false);
637-
try {
638-
workspace.prepareOperation(notificationsRule, innerMonitor);
639-
// don't open the tree eagerly because it will be wasted if no change occurs
640-
workspace.beginOperation(false);
641-
workspace.broadcastBuildEvent(Project.this, IResourceChangeEvent.POST_BUILD, trigger);
642-
// building may close the tree, so open it
643-
if (workspace.getElementTree().isImmutable()) {
644-
workspace.newWorkingTree();
645-
}
646-
} finally {
647-
workspace.endOperation(notificationsRule, false);
648-
}
644+
workspace.endOperation(notificationsRule, false);
649645
}
650-
} finally {
651-
subMonitor.done();
652646
}
653647
}
654648

0 commit comments

Comments
 (0)