Bug 551362 - [regression] monitor.done() is not called anymore on resource operations
Summary: [regression] monitor.done() is not called anymore on resource operations
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Resources (show other bugs)
Version: 4.14   Edit
Hardware: All All
: P3 critical (vote)
Target Milestone: 4.14 M1   Edit
Assignee: Karsten Thoms CLA
QA Contact:
URL:
Whiteboard:
Keywords: regression
Depends on:
Blocks: 540953
  Show dependency tree
 
Reported: 2019-09-23 03:39 EDT by Andrey Loskutov CLA
Modified: 2019-09-25 08:20 EDT (History)
3 users (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Loskutov CLA 2019-09-23 03:39:15 EDT 
We see now another severe regression in our internal tests, as a regression from bug 540953.

ResourcesPlugin.getWorkspace().build(kind, monitor) never reports done() on given monitor instance now. So code that expected that the monitor.isFinished() is true after workspace build (and that monitor.done() is called on given callback) fails now badly.

I assume all other monitor "optimizations" in platform resources also break that contract.

Please fix all the commits that removed 

finally {
    monitor.done();
}

from platform resources code.

So bad commits I see are:
* d0ce58398740f9a4dac0eb31f1c04919e3023a58
* f9b555bbce9ae279d57dff03d45e1f99aa4c82d4
* ceb13e689553b20c0fd0cde7576535a687dee865
* c1939c1f11f751d626f0ce870d3d4e39eeb81a79
* 40114d2ce9a41cbec7f01d8342d4371a6d970af0
Comment 1 Karsten Thoms CLA 2019-09-23 03:44:04 EDT 
I'll provide patches for that. SubMonitor's doc tells that done() is not required to call, but at least I think it is clearer to do so. I commented that on changes, but finally they were removed.
Comment 2 Lars Vogel CLA 2019-09-23 03:50:10 EDT 
I have a look tonight
Comment 3 Lars Vogel CLA 2019-09-23 04:02:27 EDT 
(In reply to Karsten Thoms from comment #1)
> I'll provide patches for that. SubMonitor's doc tells that done() is not
> required to call, but at least I think it is clearer to do so. I commented
> that on changes, but finally they were removed.

Te caller of the methods needs to call done(), and I guess that is missing. If you want to provide a patch please check that. Or wait until tonight so that I have the chance to check.
Comment 4 Eclipse Genie CLA 2019-09-23 04:19:26 EDT 
New Gerrit change created: https://git.eclipse.org/r/149969
Comment 5 Karsten Thoms CLA 2019-09-24 04:18:59 EDT 
I am working on enhancing the resource tests, e.g. IFileTest, WorkspaceTest. In there I am using a FussyProgressMonitor and check after each call FussyProgressMonitor#assertUsedUp(). When doing this I get assertion failures "ProgressMonitor not used up" when done() is not called on the SubMonitor in finally blocks. These assertions do not fail when done() is invoked. This leads me to the conclusion that it is indeed necessary to call done().

This also unveils some code places that do not call done() yet, but which were not reported as errors yet. E.g. Project#close() which changed 4 years ago (#645544a5df52fb3a81c7971a95aecb17f12c544d).
Comment 6 Eclipse Genie CLA 2019-09-24 05:38:56 EDT 
New Gerrit change created: https://git.eclipse.org/r/150045
Comment 9 Andrey Loskutov CLA 2019-09-25 08:20:04 EDT 
OK, I've discussed this also with Simeon, with the two last patches merged we should be OK now (monitor ticks are fully used and we have extra tests). 

We will have to update test expectations in our product, and I hope no one else relied on monitor.done() calls in resources API.

We will see if someone cries after M1, in that case we should reconsider to re-add done() calls on original monitors.

Thanks Karsten for extra tests.