Bugzilla – Full Text Bug Listing
| Summary: | [DI] The current 'uninject' scenarios break the API contract | ||||||
|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Eric Moffatt <emoffatt> | ||||
| Component: | E4 | Assignee: | Project Inbox <e4.runtime-inbox> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | john.arthorne, ob1.eclipse, pwebster, remy.suen | ||||
| Version: | unspecified | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | PC | ||||||
| OS: | Windows XP | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Eric Moffatt
Paul and Remy, what do you think? In a pure mathematical sense I like the symmetry we have now: the #uninject()/#dispose() will remove references for services that are no longer valid. It also helps OSGi bundle unload (the bundle that has any of its classes used elsewhere can't be unloaded). However, I agree with Eric that this adds some compelxity as code might need to add "null" checks even to non-optional services. From what you've seen, is there a need to reset injected values to "null" on #uninject()/#dispose()? (In reply to comment #1) > From what you've seen, is there a need to reset injected values to "null" on > #uninject()/#dispose()? If we didn't, wouldn't we potentially cause garbage to build up since we still have pointers to these things that aren't needed? A long while ago, we had the approach of disposing any object with non-optional services when one of its required services went away. I think this was one of the original reasons for IDispose API. That way, an object has two options: - all of its services must be optional, OR - it must be disposable Those conditions could be validated on first injection, to ensure we never even support an object that cannot either be disposed or support null services. I agree requiring the client to add null checks for non-optional services isn't a good solution. And, either way I agree with nulling/uninjecting a service that goes away, even for non-optional fields. Allowing a client to reference a disposed service doesn't really help them - instead of an NPE they will get some other random failure. Also it would prevent the bundle providing that service from being completely removed from the framework (reference to its class would prevent its class loader from being discarded). The only thing clear so far is that John must have a twin :-): Comment 3: > I agree requiring the client to add null checks for non-optional services isn't a > good solution. Comment 4: > I agree with nulling/uninjecting a service that goes away, even for non-optional fields. Here is what I'd like to do, opinions welcome: Summary: (1) if a context is disposed, all its injected objects get @PreDestroy calls, but we don't set injected values to "null"s; (2) if #uninject() is called directly, it will call @PreDestroy, set injected values to "null" and produce a warning if any such value is not optional. Why: The main goal of DI is to simplify developer's life. Having "null" checks does not do that, so "null" checks will have to go, at least from usual scenarios. This is not such a scary thing as it seems. Most of the time objects injected with a context have a life span limited by that of the context. Hence, when the context is disposed, the object is typically of no interest anymore and should be in the GC pool. When the object is expected to survive the context: a) its context can be reparented; or b) it could be #uninject()'ed. Hence, the suggestion (1): call @PreDestroy; make context forget about objects; expect objects to be garbage collected, which, in turn, will release services should they need to be unloaded. For special cases, #uninject() can be called directly. (It will be indeed for special cases: there are only 3 calls currently in the SDK code to ContextInjectionFactory#uninject(); at last two of those are probably not needed.) The #uninject() will replace injected values with nulls. It will be expected that any objects supporting uninject will have all injected values specified as "@Optional". === Note 1. It is possible to provide clearer distinction between objects who's lifecycle is limited by the context and those that can survive without a context: - if object is created using #make() call, it is assumed to be valid only as long as the context exists; - if object is created elsewhere and then injected, it is assumed that it can live without context. Then injection will check for all injected values to be @Optional and injection will fail if that is not the case. At the first glance I like this approach, but its implementation might result in a lot of churn in existing code. === Note 2. On ContextInjectionFactory#uninject(): I'd like to look into more details into the 3 places that use it; if it turns out to be not needed in any of those places, I'd be really tempted to remove it from API and leave #uinject() only on the lower level IInjector interface. (In reply to comment #6) > Here is what I'd like to do, opinions welcome: > Summary: > (1) if a context is disposed, all its injected objects get @PreDestroy calls, > but we don't set injected values to "null"s; > (2) if #uninject() is called directly, it will call @PreDestroy, set injected > values to "null" and produce a warning if any such value is not optional. Any comments? Going once, going twice ... Created attachment 183887 [details]
Patch
Basically, patch insures that non-optional injected values are not changed to nulls. On #uninject() we'll change only @Optional values to nulls. #dispose() won't be trying to reset values to nulls; but only will be calling @PreDestroy method.
If the value is removed from the context and it is injected in a non-optional field, a error will be logged and "null" value won't be propagated to the injected object.
When the tracing option "org.eclipse.e4.core.di/debug/injector" is enabled, warnings will be logged for #uninject() that try to reset non-optional injected values.
Patch applied to CVS Head. |