Community
Participate
Working Groups
Created attachment 221473 [details] A project illustrating the problem I have a part with which has a listener method on the contribution class. When I send a matching event through the event broker, a message is printed out. When I repeatedly close and reopen the view, the message is printed out multiple times (grows with the number of close/open cycles)
To reproduct: 1) import the attached project 2) start the application 3) select "Send Event 1" from the file menu 4) Observe: you get a single message "event 1 received" 5) repeatedly close the view and reopen it from the file menu ("Open View", 5 times does the trick) 6) Select "Send Event 1" from the file menu 7) Observe: you get multiple messages saying "event 1 received"
An interesting observation is that after the first close no events are received anymore, on the next activation there's once more only one event but when close that part once more it still receives the event!
The problem is that we are caching the extended object supplier in the ProviderHelper, there we execute an ---------8<--------- IInjector injector = InjectorFactory.getDefault(); injector.inject(supplier, objectSupplier); ---------8<--------- so we bind the injector instance to the first requestors IEclipseContext (the simply case here the first views IEclipseContext) hence when we destroy the view for the first time we appropriately call @PreDestroy in our Supplier and unregister the listeners. Subsequent calls now get an instance of our supplier which is not attached to a context at all.
well the cache explains why on the first close we unsubscribe - the real problem looks like is that: a) UIEventObjectSupplier.UIEventHandler misses the check if the requestor is valid (compare that to the DIEventHandler!) b) But even when patching it requestor.isValid() returns true although the context and object got removed (but most likely not gc'ed) I can proof this by adding a System.gc(); before the Send1Handler is calling its code and then isValid returns false! There most be something more explicit to mark the requestor invalid
Please also note haveing the @PreDestroy in the supplier is also not really a good idea because in that case we'll remove listeners registered by other injected instances as well!
To me it looks like the ExtendedObjectSupplier as it is implemented today is completely broken, because we are using DI to inject stuff. The problem is that the ExtendedObjectSupplier is a OSGi-Service hence there's only one instance of a supplier and we are filling it with the first requestors PrimaryObjectSupplier, if the first supplier is something like a part whose context is coming and going together with the corresponding IEclipseContext we are uninjecting the values (e.g. the logger in the UIEventObjectSupplier) and invoking the @PreDestroy (e.g. EventObjectSupplier). I think we need to support DI for suppliers (e.g. to get access to the UISynchronize) so we can't directly contribute ExtendedObjectSupplier instances instead probably have to register ExtendedObjectSupplier-Factory and create object suppliers for every ObjectSupplier.
Created attachment 221492 [details] Runtime modification I've added a method to IRequestor to allow the requestors PrimaryObjectSupplier to inject values so that handlers are now created through DI to get access to context informations. I'm not sure this is a good thing or there's something one can do instead certainly adding a dependency on IEclipseContext is not the correct one so I modified the IRequestor-Interface.
Created attachment 221493 [details] UI Patch here you see DI in action to inject the logger and UISync stuff
Might be related to https://bugs.eclipse.org/bugs/show_bug.cgi?id=389863
right it is what i expressed in comment 5
Created attachment 221504 [details] revised runtime patch I've modified the IRequestor to now expose the IInjector and PrimaryObjectSupplier. I think this is in general a good thing because e.g. the tools.services is currently using internal APIs to create an instance through DI inside a supplier
Created attachment 221505 [details] revised ui patch
> b) But even when patching it requestor.isValid() returns true although the > context and object got removed (but most likely not gc'ed) > > I can proof this by adding a System.gc(); before the Send1Handler is calling > its code and then isValid returns false! There most be something more > explicit to mark the requestor invalid I filed bug 390424 for the problem with isValid() if we manage to fix this we can probably make an intermediate fix in 4.2.2.
Are you talking about a fix for this bug or for #390424 in the 4.2.2 timeframe?
Well if the isValid() is fixed we can have an intermediate fix for both bugs - I guess
Oleg could you take a look at the direction i'm heading to - if is the way to go i'll write junit tests for them
(In reply to comment #16) > Oleg could you take a look at the direction i'm heading to - if is the way > to go i'll write junit tests for them I just recently got back to work on Eclipse and currently looking at the memory leaks. Once that is done I'll do my best to work on the backlog of bugs.
Oleg?
First, couple things to improve the sample project: - Getting javax annotations: Instead of Require-Bundle: ... javax.annotation please use: Import-Package: javax.annotation;version="1.0.0" ... Otherwise you risk @PostConstruct and @PreDestroy annotations not getting called. This happens due to confusion between javax annotations supplied by the OSGi bundle and JVM. - Injecting events: Instead of @Inject public void event1Occurred(@Optional @UIEventTopic...) please use: @Inject @Optional public void event1Occurred(@UIEventTopic...) The difference is that second form only gets called when an event occurs while the first form gets also called on injection and uninjection.
About multiple calls: note that those occur on different TestView1 objects. Each object gets one call (good), however, views that should have being deleted get an event too (bad). The problem lies in the Injector#uninject() call (called via PartRenderingEngine#safeRemoveGui()): ContextInjectionFactory.uninject(client, parentContext); That removes references from the primary object supplier (EclipseContext), but does not touch secondary object suppliers, such as the event supplier. Secondary suppliers currently continue to send events until views get garbage collected. The InjectorImpl has a forgetInjectedObject() method. I'd try to see if we can make similar list of objects for the secondary suppliers or make secondary suppliers refer to this list.
I'm not sure I can follow your explainations. I think the real problem in this complete supplier story here is: IInjector injector = InjectorFactory.getDefault(); injector.inject(supplier, objectSupplier); we are attaching the supplier to the initial requestor and if this guy gets disposed we uninject things like the logger, ... . Did you looked at my proposed patches?
To repeat what I said on IRC: a) object suppliers are OSGi-Services and we are attaching them to an arbitary context and its lifecycle The following problem are arising from this: * bug #389863 * uninjecting of informations like UISynchronize in UIEventObjectSupplier when initial context is disposed * Multi-Instance support e.g. RAP because there any application has to have its own UISynchronize but we have only 1 ObjectSupplier instance! b) cleaning up of listeners (e.g. EventHandler) is not done in a synchronous fashion but rely on GC running I think the general rule here is that. It is not the supplier who has to be attached to the lifecycle but the part that does the object injection (for events this is the EventHandler). This one is created within the context of the object it does the injection for. This is exaclty what my 2 patches do. Paul has set this bug to 4.2.2 but if you are following my suggestion this is not possible to fix this because we need new API (well we didn't release any of this as API but I'm not sure which rules apply).
(In reply to comment #22) > Paul has set this bug to 4.2.2 but if you are following my suggestion this > is not possible to fix this because we need new API (well we didn't release > any of this as API but I'm not sure which rules apply). We have to be careful, but we have some flexibility as we didn't publish this as API PW
The "gist" of the fix is to explicitly call #unsubscribe() in the EventObjectSupplier#get() when tracking is not requested. Git reference: http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?h=R4_2_maintenance&id=bef47180f41e3951d610fdb3dcb52eb15adce0c8 Thomas Maeder - huge thank you for providing snippets that show the issue!
I'd like to repeat that I'm not happy with the fix here and bug #389863 because it breaks multi-user envs (bug #392977). I can live with it for 4.2.2. What we have here are singletons we are brining state too which violates one of the principal design goals e4. Singletons (which OSGi-Services are!) are only fine to act as factories. I'll file a new bug now - if we could at least get the API I proposed in my patches on IRequestor I could at least fix the problem in a multi-user env by registering a higher ranked OSGi-Service.
I filed bug #392990
and bug #392977
Verified using I20121030-0800 and M20121024-1600.
While building support for @UIEventTopic and @EventTopic for the vaadin renderer https://github.com/semanticsoft/vaaclipse/issues/8 I got stuck in that I get events from one workbench to another if I use annotations. I managed to override the default event object suppliers and when I use one workbench it works ok but if I open another session I get the events from the first one. This is a killer for applications like this.
