Community
Participate
Working Groups
ServiceLocator#getService(Class) does: Object service = e4Context.get(key.getName()); if (service == null) { // this scenario can happen when we dispose the service locator // after the window has been removed, in that case the window's // context has been destroyed so we should check our own local cache // of services first before checking the registry service = servicesToDispose.get(key); } else if (service == e4Context.getLocal(key.getName())) { // store this service retrieved from the context in the map only if // it is a local service for this context, as otherwise we do not // want to dispose it when this service locator gets disposed registerService(key, service, false); } This does not handle the case where IEclipseContext.get() returns IInjector.NOT_A_VALUE, which can happen if the service is actually a ContextFunction and an exception occurred during the processing.
Oh, and this then leads to a ClassCastException as from anybody performing an IEclipseContext#get(Class): !ENTRY org.eclipse.ui.workbench 4 2 2013-06-19 12:28:46.223 !MESSAGE Problems occurred when invoking code from plug-in: "org.eclipse.ui.workbench". !STACK 0 java.lang.ClassCastException: Cannot cast java.lang.Object to com.lgc.dsaf.services.interpretation.InterpretationService at java.lang.Class.cast(Class.java:3014) at org.eclipse.e4.core.internal.contexts.EclipseContext.get(EclipseContext.java:561) [...]
(In reply to comment #1) > Oh, and this then leads to a ClassCastException as from anybody performing > an IEclipseContext#get(Class): It's not a cause: EclipseContext#get(Class) attempts to cast the NOT_A_VALUE. It should just return null, and IEclipseContext#get(Class) should note that behavioural different with #get(String).
Brian, do you have any idea for fixing this? Should we just modify the ServiceLocator to handle IInjector.NOT_A_VALUE as if it was null? PW
Do we have to fix this in both locations (prevent ServiceLocator from saving it) and add the fix in EclipseContext#get(Class)? PW
Sorry Paul, I've been away on vacation with no time for development. The fix has to be in both places. I'll have some time on Friday, if that's not too late.
(In reply to comment #5) > Sorry Paul, I've been away on vacation with no time for development. The > fix has to be in both places. I'll have some time on Friday, if that's not > too late. Friday should be fine. PW
IEclipseContext#get(String) notes that: > Returns <code>null</code> if no such value is defined or computable by this context, or if the assigned value is <code>null</code>. So the bug is in EclipseContext#get(String) in that it should be handling the case where an IContextFunction returns NOT_A_VALUE and instead return null.
Pushed a patch to Gerrit as: https://git.eclipse.org/r/15823
Fix committed to master. Backport to 4.3.1 as: https://git.eclipse.org/r/#/c/15861/
Reviewed and pushed to R4_3_maintenance: http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?h=R4_3_maintenance&id=96a4f61159c20236cf0cec30eb5cfea5f471c816
Some deeper testing reveals that I missed a key case in EclipseContext#internalGet(): the underlying ValueComputation (used to manage the IContextFunction) is created on first access and then re-used in subsequent runs. I can reproduce this in the unit tests (below). I searched for other callers to ValueComputation#get() and found EclipseContext#cachedCachedContextFunctions(), an internal method used for the context-debug view. I have pushed a revised fix: for master: https://git.eclipse.org/r/15950 for R4_3_maintenance: https://git.eclipse.org/r/15951 I'd like this to be considered for 4.3.1 RC3. a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/EclipseContextTest.java +++ b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/EclipseContextTest.java @@ -330,6 +330,10 @@ public class EclipseContextTest extends TestCase { } }); + // must call several times as the underlying ValueComputation wrapper is + // created on the first time, but re-used for subsequent calls. + assertNull(context.get("x")); + assertNull(context.get("x")); assertNull(context.get("x"));
Forgot to add that I also explicitly documented that IContextFunction#compute() can return IInjector#NOT_A_VALUE. It's not often done, but is used by PartServiceCreationFunction.
Brian, I'll leave this on the list until Paul gets back, he can make the call as to whether this goes into SR1 or SR2...
(In reply to Eric Moffatt from comment #13) > Brian, I'll leave this on the list until Paul gets back, he can make the > call as to whether this goes into SR1 or SR2... Meanwhile I have reviewed and pushed the fix in master to get some testing in that stream: http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=4267133345c2161d0948faaacd9a051d74564c78 As for whether it goes in 4.3.1 RC3, I don't have a good feel for the severity on this one. Brian what is the impact if we wait until 4.3.2 on this?
The problem manifests with values that are provided via an IContextFunction. I'm using this approach to install and configure a service that is only applicable for particular types of editor; the IContextFunction installer returns IInjector.NOT_A_VALUE for any other editors. I have a source provider that tracks the active editor via a part listener, and that requests this service for the newly active editor so as to update several variables. So switching out and back to an editor will cause at least two requests for this service from that editor. The impact of this change is that the first request for such a service from a non-applicable editor will result in a NOT_A_VALUE, which will be properly handled (that is, IEclipseContext#get() will return null). But subsequent requests take a different codepath such that IEclipseContext#get() will return the IInjector.NOT_A_VALUE object. Depending on how the caller treats the return value, this often leads to a ClassCastException, which is what occurs with ServiceLocator#getService(). So by not applying this second fix, we have only deferred the original problem, of a CCE, by one lookup. There is a sort-of work-around: have the IContextFunction return 'null' instead, short-circuiting the context lookup. Returning NOT_A_VALUE causes the context lookup to continue upwards along the context branch, which is useful in case there is a different variant of the service on the application context (e.g., a default object or master object).
(In reply to Brian de Alwis from comment #11) > + // must call several times as the underlying > ValueComputation wrapper is > + // created on the first time, but re-used for subsequent > calls. > + assertNull(context.get("x")); > + assertNull(context.get("x")); > assertNull(context.get("x")); It sounds scary/wrong to me that context.get("x") returns different results on two immediate subsequent calls. John, did you review that part when you pushed the change?
(In reply to Dani Megert from comment #16) > It sounds scary/wrong to me that context.get("x") returns different results > on two immediate subsequent calls. John, did you review that part when you > pushed the change? That problem is what is fixed with the second patch. Now subsequent calls returns the same thing -- assuming the context function doesn't change its computed value.
(In reply to Brian de Alwis from comment #17) > (In reply to Dani Megert from comment #16) > > It sounds scary/wrong to me that context.get("x") returns different results > > on two immediate subsequent calls. John, did you review that part when you > > pushed the change? > > That problem is what is fixed with the second patch. Now subsequent calls > returns the same thing -- assuming the context function doesn't change its > computed value. I see. Thanks Brian.
Released as http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=4a0c3fec26c547f64c7eab00c3f37cb2b783cf8d PW
