Community
Participate
Working Groups
This is a follow up bug report to bug 277966 ------------8<------------ !ENTRY org.eclipse.ui.workbench 4 0 2009-05-31 11:41:12.063 !MESSAGE assertion failed: Getter called on disposed observable org.eclipse.core.internal.databinding.identity.IdentityObservableSet@971a24 !STACK 0 org.eclipse.core.runtime.AssertionFailedException: assertion failed: Getter called on disposed observable org.eclipse.core.internal.databinding.identity.IdentityObservableSet@971a24 at org.eclipse.core.runtime.Assert.isTrue(Assert.java:110) at org.eclipse.core.databinding.observable.ObservableTracker.getterCalled(ObservableTracker.java:197) at org.eclipse.core.databinding.observable.set.AbstractObservableSet.getterCalled(AbstractObservableSet.java:140) at org.eclipse.core.internal.databinding.identity.IdentityObservableSet.clear(IdentityObservableSet.java:159) at org.eclipse.core.internal.databinding.property.value.MapSimpleValueObservableMap.dispose(MapSimpleValueObservableMap.java:381) at org.eclipse.core.databinding.ObservablesManager.dispose(ObservablesManager.java:129) at org.eclipse.emf.example.databinding.project.ui.rcp.views.ProjectAdminViewPart.dispose(ProjectAdminViewPart.java:144) at org.eclipse.ui.internal.WorkbenchPartReference.doDisposePart(WorkbenchPartReference.java:737) ------------8<------------
This problem is due to MapSimpleValueObservableMap allowing its internal observables to be collected. Thus when the ObservablesManager is disposed, sometimes its internal observables are disposed first which leads to the error. The fix is to wrap the creation of these objects in ObservableTracker.runAndIgnore like we do in DetailObservable<Value|Set|List|Map>.
3.5.1?
I understand that we are too late in the cycle. ... this means a customer will have to wrap all ObservableManager's into try { } catch( Exception e ) { } and legacy code might fail because this is a regression from 3.4 which could lead to memory leaks because things coming after the observable manager are not disposed appropriately? I think we should test this on 3.4 and if it fails there too then deferr to 3.5.1 else this a break code change.
Another problem with this bug is that developers might not run into the bug in development because as stated by Matt this heavily depends on the hashKey created and so the failure is random and only the deployed application might run into the bug :-(
I'm coming to the conclusion that while the implementation of runAndCollect is fine, the side effects are not fully known and are for the moment unstable. Therefore I think for the moment we must discourage using runAndCollect in the following operations: * Creating a DataBindingContext -- ObservablesManager could dispose its internal list of bindings * Observing a value property over an observable map -- ObservablesManager could dispose its internal observable map. * Observing a nested value property over an observable set -- this is a variant of the previous item There may be more but this is what I've discovered so far. This seems to be fairly pervasive problem whenever you are creating internal observables or using their tracked getters.
Created attachment 138170 [details] Patch to demo problem with an existing snippet
Would my first fix in 277966 fix the bug completely. If you simply check in the manager if a observable is already disposed you remove it from the list of items to dipose.
No, it wouldn't fix it. This is a pervasive problem brought on by the introduction of runAndCollect. Basically any code that creates an observable that is not intended to be seen or used by outside code is having that internal state snatched by runAndCollect, thus giving the receiver of those collected observables the ability to really muck around with objects they're not supposed to have access to. It's like the Patriot act, applied to databinding. All your observables are belong to ObservablesManager (even if they're supposed to be private state). I'm concerned we may have made a mistake here.
I see then we should document the runAndCollect-method to be broken (it's a new method in 3.5 anyways)
I would vote for putting something like the following in the JavaDoc: <p><em>NOTE: As of 3.5, there are unresolved problems with this API, see bug 278550. If we cannot find a way to make this API work, it will be deprecated as of 3.6.</em></p> RC4?
Created attachment 138195 [details] Javadoc disclaimer per comment #10
Created attachment 138198 [details] Added @noreference tags to discourage I'm not sure if the wording is too harsh..
Created attachment 138202 [details] Some runAndIgnore fixes to make Snippet025 run without error
Tom, Boris: Just to clarify, I am asking for review on the 3rd attachment. The 4th attachment will be handled during 3.6, I am just capturing my current state of work.
Not sure about the @noreference - I'll talk to McQ about it tomorrow.
+1 for the JavaDoc. Couldn't we catch the Exception inside the manager in 3.5.1 so that the exception is not bubbling up the stack?
Created attachment 138207 [details] Catch exception
Created attachment 138221 [details] Catch and log exception
+1 for patch #2, it's too late to discuss whether we can add @noreference at this point. Sorry for that, I was busy with other critical 3.5.0 bugs.
Released the patch (second attachment) with a minor change to list both the plug-in version and the Eclipse release number.
Created attachment 138508 [details] for my EMF-Example I've written an enhanced ObservableManager Matt & Boris do you think that this implementation is doing what ObservableManager is supposed to do?
Created attachment 138519 [details] Even simpler - this one is only collecting observable of type IEMFObservable not 100% ideal because there are left out ComputedValue, ... but for my use case it is enough.
(mass update) Removing target milestone, this bug was targeted at a milestone in the past.
I'm going through the sources project by project, and so far my impression is that ObservableTracker.setIgnore(boolean) in a try/finally block works well. For the problem at hand it is very easy to just find where the observables are instantiated and add this block to prevent those observables from being collected. However I've also been trying to add these blocks whenever an internal observable's @TrackedGetter is called, and the surface area of interfaces like IObservableList is making this a nightmare. My thinking is that to fix these we should implement some decorating observables with the ObservableTracker.setIgnore try/finally blocks in every tracked getter. Then at every class with internal observables, we simply wrap these observables in the decorator. Hopefully this make sense to everybody..
Decorator classes have drawbacks: The additional layer of indirection makes it harder to understand and debug the code, and potentially affects performance. Also, they may lead to problems with equality and hashcode. An alternative would be to write a conformance test class that checks that calls to observables' getters and constructors are properly ignored. Just throwing this out to make sure we only add decorator classes when the benefit outweighs their cost.
(In reply to comment #25) > Decorator classes have drawbacks: The additional layer of indirection makes it > harder to understand and debug the code, and potentially affects performance. On this subject, I've actually thought about exposing the property observable implementations for exactly that reason. i.e. all bean, widget and jface property observables have to be wrapped in an IBeanObservableValue, ISWTObservableValue or IViewerObservableValue decorator to to get those extra interface methods. Debugging property observables would be *immensely* easier without all those decorators. > Also, they may lead to problems with equality and hashcode. There were some problems with the decorating observables with regard to equality but I think these are resolved now. If this pattern is implemented by extending the existing decorating observables then > An alternative would be to write a conformance test class that checks that > calls to observables' getters and constructors are properly ignored. The only conformance test I can think of is to do a bunch of different operations, and expect that runAndMonitor will return just that observable and no others? This may work on observables, but it wouldn't help with non-observable classes that use observables internally, e.g. DataBindingContext. > Just throwing this out to make sure we only add decorator classes when the > benefit outweighs their cost. The benefit of using the decorators is that you have one set of observable decorators that implement the ignoring aspect correctly, instead of having to sprinkle that code all over the code base. It's no big deal for IObservableValue, but it's a huge pain for the observable collections because the interfaces are so wide and you have to wrap and protect @GetterCalled methods on iterators too.
Also, just to clarify: ignoring internal observables for @GetterCalled methods is a matter of improving runtime performance. Unless somebody is doing something very odd like disposing monitored observables, there is relatively little risk of third-party code doing damage to internal observables. By far the most important work being done here is to wrap the construction of internal observables in the setIgnore try/finally block, since runAndCollect is intended to be used for lifecycle management.
(In reply to comment #26) > On this subject, I've actually thought about exposing the property observable > implementations for exactly that reason. i.e. all bean, widget and jface > property observables have to be wrapped in an IBeanObservableValue, > ISWTObservableValue or IViewerObservableValue decorator to to get those extra > interface methods. Debugging property observables would be *immensely* easier > without all those decorators. Filed bug 282383
Created attachment 140791 [details] Wrap construction of all private observables in setIgnore try/finally block This patch only addresses the runAndCollect problem. The runAndMonitor issues will be addressed in bug 282395 which will get its own patch shortly. Tom, can you verify whether this patch fixes the issue? I know it's hard to reproduce reliably but hopefully this should fix it.
Created attachment 140795 [details] Patch Backported previous patch to R3_5_maintenance branch
(In reply to comment #28) > (In reply to comment #26) > > On this subject, I've actually thought about exposing the property observable > > implementations for exactly that reason. i.e. all bean, widget and jface > > property observables have to be wrapped in an IBeanObservableValue, > > ISWTObservableValue or IViewerObservableValue decorator to to get those extra > > interface methods. Debugging property observables would be *immensely* easier > > without all those decorators. > > Filed bug 282383 Oops, I meant to say bug 282393.
Created attachment 141253 [details] Updated patch for R3_5_maintenance branch
Created attachment 141254 [details] Updated patch for R3_5_maintenance branch Starting to get annoyed with my patches getting truncated.
Created attachment 141255 [details] mylyn/context/zip
Created attachment 141576 [details] Patch for R3_5_maintenance Saving workspace state in case there are changes I've forgotten about
Boris, can I get your +1 for the latest patch for 3.5.1?
+1
Created attachment 144271 [details] Updated patch for 3.5 Fixed contribution comments, removed whitespace-only changes
Created attachment 144272 [details] mylyn/context/zip
Patches released to R3_5_maintenance and to HEAD > 20090812
*** Bug 282395 has been marked as a duplicate of this bug. ***
Hi, Has this problem been fixed or functionality deprecated? Ref to the Indigo Doc for ObservablesManager.runAndCollect which reads: <i>NOTE: As of 1.2 (Eclipse 3.5), there are unresolved problems with this API, see bug 278550. If we cannot find a way to make this API work, it will be deprecated as of 3.6.</i> Thanks.
I think I just ran into this bug (using 3.8 as target platform): org.eclipse.core.runtime.AssertionFailedException: assertion failed: Getter called on disposed observable org.eclipse.core.databinding.observable.list.WritableList@70624c at org.eclipse.core.runtime.Assert.isTrue(Assert.java:110) at org.eclipse.core.databinding.observable.ObservableTracker.getterCalled(ObservableTracker.java:252) at org.eclipse.core.databinding.observable.list.ObservableList.getterCalled(ObservableList.java:241) at org.eclipse.core.databinding.observable.list.ObservableList.hashCode(ObservableList.java:92) at java.util.HashMap.removeEntryForKey(Unknown Source) at java.util.HashMap.remove(Unknown Source) at org.eclipse.jface.internal.databinding.viewers.ObservableCollectionTreeContentProvider$TreeNode.dispose(ObservableCollectionTreeContentProvider.java:491) at org.eclipse.jface.internal.databinding.viewers.ObservableCollectionTreeContentProvider$TreeNode.access$0(ObservableCollectionTreeContentProvider.java:489) at org.eclipse.jface.internal.databinding.viewers.ObservableCollectionTreeContentProvider.inputChanged(ObservableCollectionTreeContentProvider.java:137) at org.eclipse.jface.databinding.viewers.ObservableListTreeContentProvider$Impl.inputChanged(ObservableListTreeContentProvider.java:55) at org.eclipse.jface.databinding.viewers.ObservableListTreeContentProvider.inputChanged(ObservableListTreeContentProvider.java:203) at org.eclipse.jface.viewers.ContentViewer.handleDispose(ContentViewer.java:171) at org.eclipse.jface.viewers.StructuredViewer.handleDispose(StructuredViewer.java:2328) at org.eclipse.jface.viewers.ContentViewer$2.widgetDisposed(ContentViewer.java:214)
I'd like to remind the developers that the comment #42 is still present in the code in Eclipse 4.5. Is the issue fixed so that the comment can be removed?
I have examined the discussion here and the patches that were applied to find out if the note on method ObservablesManager.runAndCollect mentioned in Comment 42 is still valid. It seem like after the patches from this ticket were applied the note is *not* valid, and the problem it warns for seems to be solved. I create bug 565160 for the investigation of the problem and the removal of the note.
On a second thought... It is probably best to deprecate ObservablesManager.runAndCollect. More information on bug 565160.
