Community
Participate
Working Groups
Created attachment 156020 [details] Scheduler problem tests patch In 3.x, we have some simple sanity tests like the following: IWorkbenchPart part = page.showView(IPageLayout.ID_OUTLINE); assertEquals(part, page.getActivePart()); When I try to implement this in the compatibility layer, it does not work. Initially, I thought it was a bug in EPartService (bug 295003) but I have discovered that it is because the scheduled notifications have not been run so reinjection hasn't happened. Since I might not be making a lot of sense, please use the attached patch to see the problem in pure e4 code. I am marking this as major because it is an architectural issue that we need to have an answer for. It does not make sense for client code to be spinning the event loop.
Yes, just as you described ContextInjectionFactory#make() might return object that has not been injected [yet] if UISchedulerStrategy is used; same goes for ContextInjectionFactory#inject(). This is definetely a problem; I look into it after M3.
Created attachment 156024 [details] Scheduler problem tests patch I don't know why I made such a complicated test. This one is much simpler.
(In reply to comment #1) > Yes, just as you described ContextInjectionFactory#make() might return object > that has not been injected [yet] if UISchedulerStrategy is used; same goes for > ContextInjectionFactory#inject(). > This is definetely a problem; I look into it after M3. Correction, the initial injection ContextInjectionFactory#make()/#inject() does the injection right away and is not a problem. The problem you are seeing is that changes to the context are propagated asynchronously when UISchedulerStrategy is used. So, as you saw in the example, the "first" string is set right away, but the request to propagate its update to the value "second" happens asynchronously. This indeed is by design and I do agree that this presents problem for the tests. Now the question to figure out is if this is going to present a problem for adopters.
I agree with Remy that this is a problem with architecture. While [in most cases?] it is possible to refactor code to put things that need updated information into runnables themseves, that would necessitate developers understanding context event propagation. Plus, at best, the code would need to become separated into pieces based on the *proper* understanding of event propagation. So, yes, I too think that this is a major problem. Speaking with Boris, the reason behind async processing was to eliminate duplicate notifications: say, we have F(A,B), both A and B depend on X, so the dependency is F(A(X),B(X)), then whenever X changes, the F() is going to be recalulcated (and re-injected) twice. Asynchronous processing solves the problem by delaying calculations. In effect, it uses threading associated with the Display to provide a transaction model: [Phase1]: the "invalidate" events are propagated through the model in one run, all event processing is scheduled but not performed, duplicate requests are a no-op; [Boundary]: created behind the scene by Display spinning event loop [Phase2]: updates scheduled in the Phase1 are performed This is a very clever use of resources at hand now that I understand it better. (I still don't think I quite understand it :-).) However, this creates a problem described in this bug, and creates artifical dependency on the SWT/Display that should not be present in the context mechnism.
One consequence of the current approach is that we're gathering up an unbounded number of changes to be handled during a *single* asynch runnable. This is against all the UI thread rules since the UI 'hangs' during the processing of the runnable. What happens when RSA adds its 3,200 object contributions that all have to be refreshed on a selection change? Do we have to use the UI thread? Context refresh doesn't seem to be inherently UI oriented.
(In reply to comment #5) > One consequence of the current approach is that we're gathering up an unbounded > number of changes to be handled during a *single* asynch runnable. It's our current implementation, which could be changed to handle only a bounded number of changes in each runnable. > Do we have to use the UI thread? Context refresh doesn't seem to be inherently > UI oriented. This is a good question - but the result of handling changes will be to run RATs, and call setters on user objects with new values to be injected. We would need to know which ones have to be done on the UI thread, and which ones can happen on other threads.
Created attachment 159241 [details] Patch 1: reducing number of players We have 3 implementations of the Computation class with different events processing: EclipseContext.TrackableComputation; EclipseContext.TrackableComputationExt; ValueComputation My first attempts to change event propagation did not go well as those three overrides differ is subtle ways that do affect behavior (for instance, the autorelease implemented for TrackableComputation, but not for the TrackableComputationExt). So, I decided to switch to doing things in little steps; the first step to be the merge of TrackableComputation and TrackableComputationExt. While each class barely makes up half a page that too turned out to be a non-trivial task. No wonder that my original attempt to solve this in one big change did not go anywhere :-). The patch removes TrackableComputation and changes all code to use TrackableComputationExt. Two "side" bugs fixed: - UISchedulerStrategy: it is possible to get in a state where Realm.getDefault() is null. I modified code to account for this. - HeadlessApplicationTest: the context functions must only depend on information context, or they won't be recalculated properly. This is a perfect example of how subtle this is: appContext gets an MApplication added, but the aplication does not have a context set at the time. This causes computation function to fail without creating any dependencies. The context on the MApplication is set on the next line, but the previousl calculation aborted earlier and did not create a dependency on it. The fix is to reverse the order of two lines, but it took literally several hours to track this down. This is the perfect illustration that: a) calculated funtion should be used ratehr more carefully then they are now; b) calculated functions really must not depend on anything outside of context that may change c) we do need a better debug story
Created attachment 159382 [details] Patch 2 removing duplicate events This patch brings us one more step forward. In this patch we remove duplicated events and start seeing separation between "internal" context events (ValueComputation) and "external" context events (TrackableComputationExt). The "external" events are collected in the lists and processed after the wave of "internal" context updates is done. This achieves several goals: - we can eliminate duplicate events - some would-be duplicate events are eliminated naturally as dependencies are cleared for scheduled runnables - the separation between "internal" and "external" events starts to show up I added a new JUnits "InjectionUpdateTest" to test removal of duplicate notifications when UI scheduler is not used. ==== Now we have eliminated duplicate calls when no scheduler is used. At this point we can remove UI scheduler altogether and the original problem described in this bug would be solved. However, the better approach - and the approach to allow us non-trivial batching of context events - would be to introduce two classes of listeners for "external" context events: - "basic" listeners -> to be fired right away - [abstract] "batching" listeners -> such listeners would accumulate events but will only fire them when they receive a trigger call. One particular implementation could be tied to the UI event loop (idle processing?) and/or tied to a periodic "heartbeat".
I'm just porting to I20100224. The one change I found is that IEclipseContext#runAndTrack(Runnable) has disappeared as part of handling this bug. But I'm having some difficulty understanding how / what to do with the runAndTrack(IRunAndTrack, Object[]). IRunAndTrack#notify(ContextChangeEvent) returns a bool, but there's no indication what the possible values mean. So I've changed my code from: context.runAndTrack(new Runnable() { public void run() { if(composite == null || composite.isDisposed()) { return; } if(activeTaskChanged()) { updateContents(); } }}); to: context.runAndTrack(new IRunAndTrack() { public boolean notify(ContextChangeEvent event) { if(composite == null || composite.isDisposed()) { return false; } if(activeTaskChanged()) { updateContents(); } return true; } }, null); I think that's right, though I'm not sure, from the docs, what should be done with the event. The object array looks funny; could there be two variants, one with and without the array? While I'm nitpicking, I also find the documentation on ContextChangeEvent a bit puzzling. ContextChangeEvent#getEventType() is puzzling — there's no indication where to find more information; I'm assuming its one of CCE#{ADDED,DISPOSE,INITIAL,REMOVED,UNINJECTED}. And I have no idea how an object is uninjected! I'm guessing it's an object where The @Inject values are nulled? Is oldValue the uninjected object?
(In reply to comment #9) > context.runAndTrack(new IRunAndTrack() { > public boolean notify(ContextChangeEvent event) { > if(composite == null || composite.isDisposed()) { return false; > } > if(activeTaskChanged()) { updateContents(); } > return true; > } > }, null); That looks good. Wow, I am impressed, with no docs whatsoever you got it right. The return value is used to clear unneeded dependencies: true - the IRunAndTrack object wants to be notified of future changes; false - don't call this object anymore There is an open bug to add that to the doc. I haven't done it yet as IRunAndTrack is a somewhat "advanced" functionality; I am not sure if it is going to be public in the final version as injection coupled with the IContextFunction should cover [all? / most? ] of the use cases. By the way, could you describe why did you decide to use #runAndTrack() rather then just use injection? > ...ContextChangeEvent#getEventType() is puzzling ... Yes, totally agree. Same as above - this class is likely to change and is work in progress. > ... And I have no idea how an object is uninjected! ContextInjectionFactory#uninject() (I've been planning to write an article on how to use contexts & injection for a couple months now, but so far the development takes precedence.)
From comment 10: > The return value is used to clear unneeded dependencies: > true - the IRunAndTrack object wants to be notified of future changes; > false - don't call this object anymore Does that mean the if-nothing-is-checked-then-removed behaviour is no longer supported? > By the way, could you describe why did you decide to use #runAndTrack() rather > then just use injection? I'm placing my own variable into the Window's context. Views within that window react to changes to the variable. So that code snippet appears in each view: context.runAndTrack(new IRunAndTrack() { public boolean notify(ContextChangeEvent event) { if(composite == null || composite.isDisposed()) { return false; } if(activeTaskChanged()) { updateContents(); } return true; } }, null); My activeTaskChanged() checks the value, compares it to the previous value and if deemed important, causes the view to update. Is there any possibility for ContextChangeEvent to have another change type for MODIFIED, instead of (what I presume would be a) REMOVED+ADDED?
(In reply to comment #11) > From comment 10: > > The return value is used to clear unneeded dependencies: > > true - the IRunAndTrack object wants to be notified of future changes; > > false - don't call this object anymore > > Does that mean the if-nothing-is-checked-then-removed behaviour is no longer > supported? That works too, but this behavior is not obvious to clients and we've had cases where we had to play tricks in the IRAT implementation in order for removal to happen. The return value gives the client an explicit way to indicate that the IRAT is no longer needed. This is all good in theory but unfortunately in practice this isn't completely working today: in cases where the IRAT notification happens asynchronously the return value gets lost (see UISchedulerStrategy#schedule(IRAT, CCE)).
This has been fixed; UISchedulerStrategy is replaced with "@GroupUpdates". (In reply to comment #11) > Is there any possibility for ContextChangeEvent to have another change type for > MODIFIED, instead of (what I presume would be a) REMOVED+ADDED? I strongly suspect that both IRunAndTrack and ContextChangeEvent will be internal in the released version: dependency injection and context functions (IContextFunction) should cover [almost?] all use cases for tracking context changes.
