Bugzilla – Full Text Bug Listing
| Summary: | BuildManager#basicBuild makes trees immutable() in finally | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | James Blackburn <jamesblackburn+eclipse> | ||||||||
| Component: | Resources | Assignee: | James Blackburn <jamesblackburn+eclipse> | ||||||||
| Status: | CLOSED WONTFIX | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | john.arthorne, Szymon.Brandys | ||||||||
| Version: | 3.7 | ||||||||||
| Target Milestone: | --- | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | stalebug | ||||||||||
| Bug Depends on: | 343256 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
I think this would be fixed by bug 343256 where the tree is made mutable before relinquishing the WS lock when invoking the builder. (A new working tree is also already created when the top-level build operation ends.) In Bug 343256 buildManager creates a newWorkingTree() before releasing the WS lock. I'm reopening the bug since RC1 requirements were not fulfilled. See http://www.eclipse.org/eclipse/development/plans/freeze_plan_3_7.php The end-games rules are: 1) +1 from another committer 2) a patch attached reviewed by another committer (In reply to comment #3) This was an (likely rare) issue I noticed by inspection. The issue was fixed coincidentally as part of Bug 343256 which was reviewed. Sorry, I'm off till the end of the week, so maybe John will take a look at it earlier. (In reply to comment #4) > (In reply to comment #3) > This was an (likely rare) issue I noticed by inspection. > The issue was fixed coincidentally as part of Bug 343256 which was reviewed. I see. I would mention such details, especially during RCs. We also need a test. I talked to James and he will attach one soon. I have reviewed it. Created attachment 194782 [details]
WIP fix + test
It was worth writing a test...
With relaxed builders the bug exists since 3.6 and still in 3.7. BuildManager must ensure the working tree is open before beingUnprotected.
We must also ensure the tree is not immutable before ending the top-level build operation from Project and Workspace build. Otherwise we'll get the attached exception and the test will fail.
This issue has existed since 3.6, so it must be pretty rare that it hasn't been reported up till now.
java.lang.RuntimeException: Illegal attempt to modify an immutable tree.
at org.eclipse.core.internal.dtree.AbstractDataTree.handleImmutableTree(AbstractDataTree.java:248)
at org.eclipse.core.internal.dtree.DeltaDataTree.createChild(DeltaDataTree.java:329)
at org.eclipse.core.internal.watson.ElementTree.createElement(ElementTree.java:182)
at org.eclipse.core.internal.resources.Workspace.createResource(Workspace.java:1369)
at org.eclipse.core.internal.resources.Workspace.copyTree(Workspace.java:1094)
at org.eclipse.core.internal.resources.Workspace.move(Workspace.java:2114)
at org.eclipse.core.internal.resources.ResourceTree.movedFolderSubtree(ResourceTree.java:595)
at org.eclipse.core.internal.resources.ResourceTree.standardMoveFolder(ResourceTree.java:1004)
at org.eclipse.core.internal.resources.Resource.unprotectedMove(Resource.java:1966)
at org.eclipse.core.internal.resources.Resource.move(Resource.java:1591)
at org.eclipse.core.internal.resources.Resource.move(Resource.java:1559)
at org.eclipse.core.tests.resources.regression.Bug_343569$3.run(Bug_343569.java:149)
at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54)
Created attachment 194817 [details]
fix + tests 2
Both build & notification may close the tree. As the top level build has a begin and endOperation pair around the POST_BUILD notification, the tree needs to be made mutable both at end of build and end of notification (if other jobs are checked into the workspace). The test catches both these issues.
I've moved the logic to #endOperation which can make the best decision about whether to leave the tree immutable, or open.
As the bug wasn't fixed by 343256, pinging John for additional review + comment. Created attachment 194821 [details]
fix + tests 3
Beef up test to test Workspace#buildInternal too.
I like the approach but the change in endOperation worries me a bit this close to the release. I'm still trying to convince myself that is safe to reopen the tree in all cases where !depthOne. I'll review it again tomorrow. The comment in endOperation about tree.immutable() being unnecessary because of the notification manager cleanup isn't right.. if we got into the finally via an exception the notification may not have occurred. (In reply to comment #12) > I'm still trying to convince myself that is safe to reopen the > tree in all cases where !depthOne. I'll review it again tomorrow. One way is to consider: as soon as workManager.checkout() is called the WS lock is released. At this point a pending operation blocked on WS lock begins and immediately calls newWorkingTree(). The notification job is a good example, as it can exercise the full #beingOperation / #endOperation as a nested operation during the beingUnprotected of a build, or some other workspace job. As the NotifyJob always fires a resource change delta, the tree becomes immutable() / new...(). Given this, it's always safe for a newWorkingTree() to be opened while the lock is held in a nested operation. > The comment in endOperation about tree.immutable() being unnecessary because of > the notification manager cleanup isn't right.. if we got into the finally via > an exception the notification may not have occurred. Really? #endOperation can only throw CoreException if core.resources is shutdown (and this doesn't call this finally). If it were to throw a RuntimeException or Error then there seem to be two issues: 1) it would mask any CoreException or OperationCanceledException thrown by the operation itself (as endOperation is always called from finally) 2) There's nothing to catch and log the runtime exception. AFAICS if #endOperation ever throws, then the workspace is broken and all bets are off. Am I missing something? Sigh. Inspecting the #immutable call-hierarchy, there's a similar problem in IncProjBuilder#getDelta(). (In reply to comment #12) > I like the approach but the change in endOperation worries me a bit this close > to the release. On a balance of risk, I'd be happy to wait for early 3.8 to fix this issue. I've never seen this exception reported and it's been around since 3.6 AFAICS to provoke the issue the tree must be left closed during a #beginUnprotected in the middle of a nested operation. call-hierarchy shows that this would only interfere with a concurrent: delete() & move(). (In reply to comment #14) > On a balance of risk, I'd be happy to wait for early 3.8 to fix this issue. > I've never seen this exception reported and it's been around since 3.6 I think the fix is probably good. But, considering the failure case is quite rare and this is changing some pretty fundamental code paths, I think waiting for early 3.8 is the better answer. This is the kind of change where I know my ability to predict bad side-effects is not high. On the subject of exceptions during endOperation: yes, if a RuntimeException or Error occurs in the finally block we're probably in bad shape. An Error thrown by a resource change listener is perhaps one of the few cases where we might be able to recover. This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie. |
basicBuild: private void basicBuild(int trigger, IncrementalProjectBuilder builder, Map<String,String> args, MultiStatus status, IProgressMonitor monitor) { Makes the current workspace ElementTree immutable in the finally, even though the build is in an operation. This may be ok for WS Rule builders. However for builders with relaxed scheduling rules, bad things may happen as another operation may be checked in & currently running in a beginUnprotected.