Bugzilla – Full Text Bug Listing
| Summary: | [DI] OSGi services aren't injected if they weren't registered when the context is created | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Simon Chemouil <eclipse> |
| Component: | Runtime | Assignee: | platform-runtime-inbox <platform-runtime-inbox> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | bokowski, eclipse, gunnar, jawr, john.arthorne, kai.toedter, ob1.eclipse, pwebster, remy.suen, tjwatson, tom.schindl |
| Version: | 4.1 | ||
| Target Milestone: | 4.2 M4 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Attachments: | |||
After analysis, it appears that the OSGiContextStrategy indeed disregards any injected OSGi Service that is not present as seen on the last line of this OSGiContextStrategy:150
public boolean containsKey(String name, IEclipseContext context) {
// first look for a registered IContextFunction matching the name
if (getContextFunction(name) != null)
return true;
// next, look for a matching service
return bundleContext.getServiceReference(name) != null;
}
In a fully dynamic OSGi environment, we cannot rely on the fact that the ServiceReference is not null at the time the lookup() method is called because it might be registered as a service much later (at any time).
The only completely open implementation I can think of is simply:
public boolean containsKey(String name, IEclipseContext context) {
return true;
}
(After all, anything the user asks for might eventually be registered as an OSGi service).
This works fairly well and behaves as I would expect.
Unfortunately in my tests the "null" value is passed as an argument even if when the service is not set @Optional.
I believe OSGi services, considering their dynamic nature, should always be set as @Optional (an alternative would be to dispose the injected object if the service is gone, but it seems overly complicated).
About alternative implementations for an OSGi lookup strategy, one could think the user could pass a list of services that are to be considered as fully dynamic that would be waited on indefinitely.
For instance :
public boolean containsKey(String name, IEclipseContext context) {
// first look for a registered IContextFunction matching the name
if (getContextFunction(name) != null)
return true;
// next, look for a matching service
return (bundleContext.getServiceReference(name) != null) || dynamicServices.contains(name);
}
with an API in an "ConfigurableOSGiContextStrategy":
/**
* Make the Lookup Strategy wait indefinitely for this service
* @param serviceInterface the interface provided by the service to
* be waited on indefinitely
*/
public void addDynamicService(Class<?> serviceInterface);
/**
* Remove the Lookup Strategy wait indefinitely for this service
* Ignores if the service was not added
* @param serviceInterface the service interface provided by the service
* to stop waiting on indefinitely
*/
public void addDynamicService(Class<?> serviceInterface);
However I don't know:
* whether this is more practical at all, as I'm not sure not waiting for all services makes sense with an "OSGi" strategy
* how the user should add those services (early enough so that it's set before injected objects are created). I guess this strategy could itself be an OSGi service started early (?).
I'm not a big fan of this approach.
In any case, I think the current "OSGiContextStrategy" limits the OSGi programming model too much by only managing services already present when the target object is injected.
For now, my prefered approach would be "return true;" for the containsKey method and solving the @Optional bug.
(In reply to comment #1) > with an API in an "ConfigurableOSGiContextStrategy": > > /** > * Make the Lookup Strategy wait indefinitely for this service > * @param serviceInterface the interface provided by the service to > * be waited on indefinitely > */ > public void addDynamicService(Class<?> serviceInterface); > > /** > * Remove the Lookup Strategy wait indefinitely for this service > * Ignores if the service was not added > * @param serviceInterface the service interface provided by the service > * to stop waiting on indefinitely > */ > public void addDynamicService(Class<?> serviceInterface); Of course the second method should read: public void removeDynamicService(Class<?> serviceInterface); Created attachment 172526 [details]
Simple fix (more open strategy, unfortunately sends null even without @Optional)
I added the patch for the working solution described in my comment earlier.
Yes, this is a known limitation and the current code in containsKey was intentional. Your patch will probably work but I believe will have terrible performance. Any time someone looks up *any* value in any child context we will end up registering a service tracker for it, which is relatively heavy-weight. Bug 306837 is somewhat related. I realize there is a problem here but I think a solution with reasonable performance is a bit more complicated. Some possibilities: - Perhaps a single ServiceListener would be more efficient than N ServiceTracker instances - If the strategy could distinguish a simple IEclipseContext#get from an injection (run and track), then it could avoid creating a service listener for the simple lookup case. (In reply to comment #4) > Yes, this is a known limitation and the current code in containsKey was > intentional. Your patch will probably work but I believe will have terrible > performance. Any time someone looks up *any* value in any child context we will > end up registering a service tracker for it, which is relatively heavy-weight. > Bug 306837 is somewhat related. > > I realize there is a problem here but I think a solution with reasonable > performance is a bit more complicated. Some possibilities: > > - Perhaps a single ServiceListener would be more efficient than N > ServiceTracker instances > - If the strategy could distinguish a simple IEclipseContext#get from an > injection (run and track), then it could avoid creating a service listener for > the simple lookup case. Thanks for your reply. I was aware that my proposal was suboptimal and in fact I later found it was also problematic with the current code base (for instance, if a class was not found in the context, it was cached as a null OSGi service reference and it was not found when it was coming from the "legitimate" context. I lost my CTabRenderer this way). I also spent some time thinking about your comment and the consequences in terms of performance. It's true we can't have an OSGi service tracker for every class that can't be satisfied, that would be indeed a huge waste of performance. I looked into making IEclipseContext's get & runAndTrack use different lookups but I think this was not enough either, because there maybe be clashes with the @Inject anyway (like it happened with the CTabRenderer). After thinking it some time, I came to the conclusion (at least for now) that it would be better for the user to specify himself which values to be injected must be tracked even when not found. After thinking about different approaches, I ended up implementing a new annotation: @Dynamic. Roughly, any value annotated with @Dynamic (I did not make it work on IRequestors as I'd rather have some discussion with you and other interested parties before spending more time on it). I first considered an @OSGiService annotation but I did not want to create an OSGi specific API; @Dynamic, as I implemented, could very well be implemented by another dynamic provider (e.g, another service framework similar to OSGi, or simply a context strategy where some unknown values my pop later). @Dynamic is implemented like @Optional ; as there is no inheritance between annotations, I made sure that @Dynamic is considered to be @Optional at most places. Another approach would be to have parameters in @Optional (ie, @Optional(Dynamic=true) ) but I did not want to change the current @Optional implementation. *I made sure that my new code did not change any existing behavior E4 is relying on*. With my patch, the new code for the LogService problem is: class View { LogService logger = null; @Inject public synchronized void setLogService(@Dynamic LogService pLogger) { if (pLogger != null) { this.logger = pLogger; System.out.println("Logger is here"); } else if (this.logger != null && pLogger = null) { this.logger = null; System.out.println("Logger is gone"); } else { System.out.println("Logger is not there yet"); } } } And it works as expected. Do we need a new annotation? In fact I was opposed to it at first (partly because I'm already using in another project another JSR-330 framework with an OSGi provider, that is Google Guice+Peaberry, and it got rid of its @OSGiService annotation some time ago). The thing is, in Guice Peaberry, you have to "bind" the services you will be able to use later (dynamic or not) before having them in your context: there is no "open-ended" context. In E4, providing now the same kind of configuration class (in Peaberry, it's a Configuration Module class that somewhat complements/replaces the bundle activator) is late and not really in the spirit of the framework. Also, the current framework works very well for all other use cases. The @Dynamic annotation simply brings that configuration to the injected method that needs an OSGi service. It is similar in spirit, but it is also interesting because it "overrides" @Optional by making it clear that something Dynamic is also by definition Optional: it can come, go, or not be there at all. In terms of performance, there is no more problem since the ServiceTrackers will only be registered for services that are either already here, or that are explicitely marked as @Dynamic in the user code. I'll detail my implementation in the next comment with my patch: please comment. Also, if there is enough positive review and since the API is provisional anyway, it would be great to have it (or a later iteration of it) in the 1.0 release even if I realize it's very late. On the positive side, it does not break anything (I'm confident), on the down side the API has changed a bit (but no backwards compatibility, so since we don't guarantee forward compatibility for this release it might just be OK). Two interfaces have changed: IEclipseContext and ILookupStrategy, both in e4.core.contexts with two new methods. Created attachment 172597 [details]
@Dynamic annotation and OSGiContextStrategy support
I'll describe the implementation here:
As I explained in my previous post, I added the @Dynamic annotation (copied from @Optional) and made sure that it was considered as @Optional for all practical purposes so that "null" can be set when the service is not there, instead of throwing an exception.
Then, in the ContextObjectSupplier class, I made sure that RunAndTrack calls are handled differently: they now forward an array of IDescriptorObject instead of the array of keys so that I can check if the object is annotated with @Dynamic in the RunAndTrack instance, and also to keep the Class<?> object handy (for @Dynamic, having the class object is useful later on).
The RunAndTrack instance is an inner-class of ContextObjectSupplier, ContextInjectionListener. Aside from making this class aware of the descriptors rather than just the keys, I changed the update method so that:
- before calling "containsKey" (the traditional "get" method), it checks if there is the @Dynamic annotation and calls on the context the new method containsDynamic
- if it can call it with the Class<?> object, it does, if it cannot it will call with the key (so that @Named may be used by other lookup strategies)
These two new methods containsDynamic in IEclipseContext (overloaded with String and Class<?> variants) should return true if the requested item may be available at a later point.
The implementation in EclipseContext does not check for dynamic values with the "default" strategy / locally because it is not supported, rather it forwards the request to the first parent item able to resolve it.
We end up in the rootContext which has the OSGiContextStrategy, and we call the similar containsDynamic methods (also overloaded with Class<?> and String) on the strategy. The OSGi strategy returns true on the Class<?> variant, and false on the String variant, because OSGi services might only be classes. This prevents this strategy from working with @Named, but this is not a big deal because @Named is generally useless in this use case. This can be discussed, though, but my goal was to be sure I never create a service tracker on something that is not an actual class.
Even though my opinion might evolve, I currently think this is the most straightforward/safe approach at this point; the API change is minimal and the performance is good.
The @Dynamic annotation is also more pleasant to use than @Optional according to my colleagues and I agree it is clear and mimics the dynamic-mode of Declarative Services.
An interesting improvement that would break the API again would be to be able to provide LDAP filters in the @Dynamic annotation to filter on OSGi properties. Another cool thing Peaberry does is to allow @Inject Iterable<ServiceInterface> to inject all available services that implement the said interface, but this is a much bigger update.
Thoughts and comments welcome!
(In reply to comment #5) > On the positive side, it does not > break anything (I'm confident), on the down side the API has changed a bit (but > no backwards compatibility, so since we don't guarantee forward compatibility > for this release it might just be OK). Arg. It should read "no backwards compatibility" *breakage*. This patch is 100% backwards compatible :). Thanks Simon, great points and good ideas. I only had a quick look as we are swamped with items to fix for the release. Couple points: - Currently OSGi contributions are resolved by a special context implementation. A cleaner solution (both to the tracking problem and from the architectural side) is to make it an extended provider and add an "@OSGi" annotation. That will allow to remove several things that exist solely to support OSGi context. - The "@Dynamic" annotation is an interesting proposition. From what I've seen so far, it is not clear to people if injected values are updated when context changes. Part of it is education, part is things like constructor injection that behave differently. However, from the limited use I've seen so far the default intention seems to be "dynamic=true". So, if anything, the annotation should be the opposite, something like "@Final". Either way, this will be post-1.0 release item due to the need to modify e4 code using context injection. (In reply to comment #8) > Thanks Simon, great points and good ideas. > > I only had a quick look as we are swamped with items to fix for the release. > Couple points: Hi Oleg and thanks for your reply. I understand that you guys are quite busy so I appreciate you took the time to discuss this. > - Currently OSGi contributions are resolved by a special context > implementation. A cleaner solution (both to the tracking problem and from the > architectural side) is to make it an extended provider and add an "@OSGi" > annotation. That will allow to remove several things that exist solely to > support OSGi context. I agree this would be the best approach if we were completely OSGi agnostic, but E4 is in practice strongly dependent on OSGi and since the OSGiContextStrategy was already set in the rootContext it seemed to me the best and only viable approach at this time. I think ExtendedObjectSuppliers are great for annotations like @Preferences or @EventTopic, but those are somehow "add-ons". OSGi services look-up seems to me very much at the core principles of E4 for now and some time to come. Eclipse being tied to OSGi as it is, I feel it makes sense to have the OSGi service model at E4's core and not as an extension. Yet, I wanted to avoid an @OSGi annotation because a good coding principle when using OSGi is to keep the framwork details as far as possible within the implementation details: the @Dynamic annotation does just that by saying "this object might currently be out-of-scope, but don't give up for so little, get it for me using the strategy that's set up in my context". It is not saying "use OSGi to get it", just "don't give up on it if it's not there yet". > - The "@Dynamic" annotation is an interesting proposition. From what I've seen > so far, it is not clear to people if injected values are updated when context > changes. Part of it is education, part is things like constructor injection > that behave differently. However, from the limited use I've seen so far the > default intention seems to be "dynamic=true". So, if anything, the annotation > should be the opposite, something like "@Final". There are two things here, just like there are two nuances to the word "dynamic". The education of @Inject users is a problem, but not the one I'm trying to address. Sure, in essence, @Inject is "dynamic", but in the sense that it will be called again when values change, not in the meaning I'm using for the annotation. The meaning I'm using is that we're using an object to be injected that not only may be totally out of the scope when we want to inject it, but its *key* itself is unknown in any context. I'm not saying this is the best approach, but it makes sense. With my patch as it is, if you're using an OSGi service like you were before with @Inject (with or without @Optional), it will work just the way it did before. Like before, if you don't set it as @Optional this is not very clean because it does not take into account that it might be stopped, but to simplify things a lot of code does not manage this in Eclipse currently (eg, what happens if I stop the EPartService ?). To be clear, I did *not* change the previous behavior at all, even with OSGi services. What I did is provide a way for users to specify that some objects you want to be injected might not be there at all when the injector runs, and it's up to the strategy to decide what to do with it. The OSGiContextStrategy decides to track them, another lookup strategy might do something else entirely. > Either way, this will be post-1.0 release item due to the need to modify e4 > code using context injection. In fact, no code would have to be changed because the old behavior is strictly the same if you don't use the @Dynamic annotation. The patch just adds a feature for users who have dynamic services (ie, a service that's registered after the object is injected), I'm not sure we are many users but this is a valid use case as long as E4 adopts the OSGi service model (which I think is a good thing ;)). Thanks again! (In reply to comment #9) > In fact, no code would have to be changed because the old behavior is strictly > the same if you don't use the @Dynamic annotation. The patch just adds a I forgot that ILookupStrategy is not @noimplement even if it is currently in an internal package (org.eclipse.e4.core.internal.contexts) ; IEclipseContext is @noimplement As far as I know, the only ILookupStrategy implementation provided in the E4 SDK is OSGiContextStrategy but I realize that there may be a breakage if some other party already made its own ILookupStrategy. I'm not sure whether anyone did this, but providing the old behavior is simply a matter of returning false for the two new methods and my subjective guess is that anyone who was crazy enough to implement this interface at this stage would not mind two more methods :). Anyway, just to point out the fact that there might be indeed a slight change required that I did not think of. The runtime behavior is not changed though. Created attachment 172733 [details]
@Dyn
Created attachment 172735 [details] @Dynamic annotation using an ExtendedObjectSupplier (Sorry for previous reply, I hit the Enter key at the wrong time). Here's a new patch that does exactly the same but differently, as Oleg proposed. This time I am not touching API and I just added a @Dynamic annotation in org.eclipse.e4.core.di.extensions Info & thoughts: - I have not touched the OSGiContextStrategy at all so everything will work like it used to; and we will get OSGi services from this strategy when they are already present. Maybe a next step would be to remove this strategy as Oleg said to fully replace it with the proposed annotation. For the record, I tested to disable the OSGiContextStrategy for some OSGi services and @Execute, @Inject all work with the extended supplier. Of course a large part of E4 depends on services to be injected like they are now (with no special annotation) so we can't disable OSGiContextStrategy now - This implementation is direct/simple. It works, seems OK, it but might be interesting to review it. As John suggested, it registers one (1) serviceListener and listens for all services ; when one of these services changes status, and if it was requested, a new value is injected. - I had to catch an IllegalStateException when calling IRequestor#resolveArgs because OSGIServiceContext calls its bundleContext (line 158) even after it's not valid anymore, and so there is a problem there when we stop the application. - I chose the name @Dynamic because nothing prevents this annotation to be processed by another object supplier afaik and I don't like to tie it to the name OSGi. - I made a ServiceReference cache to avoid querying already known references and avoid reproducing bug #306837. For my project I'll roll the annotation in another bundle, but I provide a patch for di.extensions here so that it can be integrated with the distribution at a later point. Hope this helps, Simon Created attachment 172739 [details]
@Dynamic annotation using an ExtendedObjectSupplier (ver 2)
Code cleanup and potential problem when deactivating the component without its bundle (removing the ServiceListener).
I'd really like to see support for this in 4.0 because it makes us perfect OSGi-Service-citizens. IMHO getting so easy access to OSGi-Services is one of the best things when I showing the power of e4 to people. I'm not familiar with the injection code so I can't say how dangerous it is to bring this in but if I got the conversation right one has to put the system explicity into a tracking state for a dynamic service. (In reply to comment #14) > I'd really like to see support for this in 4.0 because it makes us perfect > OSGi-Service-citizens. IMHO getting so easy access to OSGi-Services is one of > the best things when I showing the power of e4 to people. > > I'm not familiar with the injection code so I can't say how dangerous it is to > bring this in but if I got the conversation right one has to put the system > explicity into a tracking state for a dynamic service. My vote would be to not work on this for 4.0 because you already get the main benefit, being able to access OSGi services easily. Handling the dynamic case correctly looks like it's going to be non-trivial, and I would classify it as an "icing on the cake" feature. I agree that it would be great to have, but you can still get a lot of value from what we have now. The let me rephrase this. Can people who'd like to use this dynamic stuff simply by dropping in the ExtendedObjectSupplier from Simon in an extra bundle and hurray they have dynamic support (from the last patch to me this like it would work). If we don't provide stuff make it possible for people to plug-in an experiment with stuff like this I think we won't get it right. (In reply to comment #16) > The let me rephrase this. Can people who'd like to use this dynamic stuff > simply by dropping in the ExtendedObjectSupplier from Simon in an extra bundle > and hurray they have dynamic support (from the last patch to me this like it > would work). > > If we don't provide stuff make it possible for people to plug-in an experiment > with stuff like this I think we won't get it right. Indeed, it's possible: that's what I'm doing at work. Since I don't want to manage my own fork of E4 (and keep up with updates), I have rolled the supplier in another bundle and it's working just fine. If it ever makes it in the E4 distribution, it's just a matter of renaming an import and it will work. I annotate all my services with @Dynamic, and they are provided by the OSGiContextStrategy if they're already there, or by my supplier if they aren't. It's transparent from the client side, and I make sure I can handle services coming and leaving. After careful thinking I now share Oleg's opinion that the current approach is not a good long term solution. I still think it should be supported out of the box, but the problem to have it as the root context's strategy as it is now is that the DI has different evaluation phases and the suppliers kick later if the object was not found earlier. Looking for dynamic OSGi services is fundamentally an "open" strategy, and it has to take place in a later phase (either that, or we either have to cache every failed query and refresh the requestors if a dynamic service came using a Service Listener, but afaik the current API doesn't allow to know whether a descriptor was satisfied at a later phase). To me, the Eclipse context should keep its spirit of a finite, tree-structured data directory and remove the current OSGi lookup strategy because it's not complete. The BundleContext (of the appropriate bundle, through some kind of introspection/hack, because BundleContexts aren't supposed to be shared outside of their bundle) should be available in the Eclipse context for people who want to interface with OSGi programmatically rather than through injection. Obviously this can't be done now because a lot of code within the framework expects OSGi services to be provided through context calls. So for now, the only solution is to use the Extended Supplier approach. Maybe a good solution would be to provide it in a separate bundle on GitHub and reference it in the wiki for those who need the feature. (other solutions include the incubator and Eclipse Labs. I'm fine with either but I prefer the E4 incubator because I'd rather avoid using Google Code altogether). Thanks for your comments, Simon! (In reply to comment #17) > So for now, the only solution is to use the Extended Supplier approach. Maybe a > good solution would be to provide it in a separate bundle on GitHub and > reference it in the wiki for those who need the feature. (other solutions > include the incubator and Eclipse Labs. I'm fine with either but I prefer the > E4 incubator because I'd rather avoid using Google Code altogether). Not sure what the easiest way is to do this... Simon, would you be interested in working on this aspect in the future as well? We could nominate you as a committer on e4 in that case (you'd have to write an email to e4-dev stating your interest and which area you intend to work on). In any case, we'd have to get your contribution IP-reviewed which will take a little bit of time. (In reply to comment #18) > Thanks for your comments, Simon! > Not sure what the easiest way is to do this... Simon, would you be interested > in working on this aspect in the future as well? We could nominate you as a > committer on e4 in that case (you'd have to write an email to e4-dev stating > your interest and which area you intend to work on). In any case, we'd have to > get your contribution IP-reviewed which will take a little bit of time. Hi Boris, Indeed, I will continue working on these aspects in the unforeseen future since I am currently heavily using OSGi for a large industrial project (that well keep going for several years) and we're rolling an E4 RCP application for which we require this support. We're planning to share this infrastructure with others. So I'm very much committed to keep this working. I appreciate your proposal to become an E4 committer (since there are other areas I'm interested in as well) and I'll write that email in the upcoming days. Thanks again, Simon Can we put this on the 4.2 plan? Created attachment 205838 [details]
Patch
The patch changes OSGi context to track all requested OSGi services.
By itself, this should not cause much overhead and might even be a slight improvement [for SDK use case] as we only register one service listener instead of a separate service tracker for each service provided.
As far as "extra" calls go, we only get about ~150 unique inquiries at this level in the SDK, so, again, should not be a concern.
I removed context lookup strategy and implemented new code as a super-class of the EclipseContext. This allows for cleaner and more natural integration with the context dynamic functionality.
I also used this as an opportunity to clean up warnings in the bundles I touched.
=====
One remaining issue has to do with bundle unloading.
The OSGi service has a sort of "reference counter" that tracks its usage. As long as the service is "used", its bundle can not be removed from the OSGi framework.
In the context we can not differentiate between two scenarios:
- consumer requested a service to be used in this method call and discarded, or
- consumer requested a service which he is going to hold on to for future use.
To be on the safe & simple side, for the eclipse OSGi context we'll assume that we need to hold on to the services we provided. We will release services when the OSGi context is disposed.
For consumers who live in a more dynamic environment, we'll add an "@OSGi" annotation with an argument to specify if provider should hold on to the service.
I plan to release the patch early in M4 after a bit more testing. (In reply to comment #21) > One remaining issue has to do with bundle unloading. > > The OSGi service has a sort of "reference counter" that tracks its usage. As > long as the service is "used", its bundle can not be removed from the OSGi > framework. > > In the context we can not differentiate between two scenarios: > - consumer requested a service to be used in this method call and discarded, > or > - consumer requested a service which he is going to hold on to for future use. Another problem is regarding permissions. Using one single BundleContext to bind services makes impossible to use Java security to allow or disallow the usage of a service by a given bundle, since it will always be using the BundleContext of org.eclipse.e4.core.contexts. This is quite used in the OSGi world and some RCP applications use this mechanism. I'm not sure the overhead of registering one ServiceListener (or ServiceTracker) by bundle which requests a @Inject to an OSGi service vs. using one global tracker as proposed in the patch is worth ignoring OSGi's "rule of thumb" in our case, because E4.core.contexts is truly a central component in the new-gen Eclipse stack and I'd personally go for the "blessed OSGi way". (In reply to comment #23) > > Another problem is regarding permissions. Using one single BundleContext to > bind services makes impossible to use Java security to allow or disallow the > usage of a service by a given bundle, since it will always be using the > BundleContext of org.eclipse.e4.core.contexts. This is misleading. Using another bundle's BundleContext to obtain a service does not grant the caller of BundleContext.getService() the same service permissions as the bundle of the BundleContext. A permission check is done against the call stack at the time BundleContext.getService is called. DI engines must do a separate permission check to ensure the bundle they are injecting the service into actually have the permission needed to access the service. Please see section 112.9.3 for a description of how the DS Runtime must handle permissions: Using hasPermission SCR does all publishing, finding and binding of services on behalf of the component using the Bundle Context of the component’s bundle. This means that normal stack-based permission checks will check SCR and not the component’s bundle. Since SCR is registering and getting services on behalf of a component’s bundle, SCR must call the Bundle.hasPermission method to validate that a component’s bundle has the necessary permission to register or get a service. (In reply to comment #24) > This is misleading. Using another bundle's BundleContext to obtain a service > does not grant the caller of BundleContext.getService() the same service > permissions as the bundle of the BundleContext. A permission check is done > against the call stack at the time BundleContext.getService is called. DI > engines must do a separate permission check to ensure the bundle they are > injecting the service into actually have the permission needed to access the > service. Thanks Thomas for the precision, I was indeed not very clear. I see two separate issues: (1) binding the service to the proper context (so that the framework knowns which services are consumed by which bundle) (2) ensuring that bundle permissions are respected (and cannot be escaped using E4 DI). My understanding is that the proposed patch solves neither in an attempt to limit the number of service trackers/listeners. The reason is that when requesting @Inject on a method or a field in E4, there is no hint in the code to tell the framework if it is an OSGi service or not (like an E4 Context Value or something from another container altogether) and thus the "old" code ended up registering many trackers for things that ended up not being OSGi services at all. I'm a proponent of requiring a specific configuration element (such as a binding table that binds injected types or @Named to different lookup strategies, supplied to the injector at its creation, or by an annotation in the code) to specify which are OSGi services. The default lookup strategy could be similar to the current (before patch) solution, that is, look in context then look in the OSGi service registry but consider it optional (ie, if there's a satisfying service return it and track it, otherwise return null and don't try to resolve it again). This way we could be backwards compatible with the current code. I also read in the meeting minutes that Sisu was being investigated as a possible replacement for E4 DI. As far as I know it handles OSGi services properly and it might be interesting for reuse among Eclipse projects. I couldn't find any bug open about that, but that would be an interesting direction. (In reply to comment #26) > I also read in the meeting minutes that Sisu was being investigated as a > possible replacement for E4 DI. As far as I know it handles OSGi services > properly and it might be interesting for reuse among Eclipse projects. I > couldn't find any bug open about that, but that would be an interesting > direction. That too is misleading :-) As far as plans go, there are no plans to switch e4 to Sisu. I can add my personal view on this. Sisu, as far as I know, is a {modified Guice} + {modifed Guava} + {Sisu code}. Personally, I think it would be great to see it we can combine e4 contexts and dynamic injection with Guice. This is not going to happen this release, but, I hope to be able to work in this direction. If nothing else, this eliminates duplication of efforts in the long term. (In reply to comment #27) > That too is misleading :-) > > As far as plans go, there are no plans to switch e4 to Sisu. > > I can add my personal view on this. Sisu, as far as I know, is a {modified > Guice} + {modifed Guava} + {Sisu code}. Personally, I think it would be great > to see it we can combine e4 contexts and dynamic injection with Guice. This is > not going to happen this release, but, I hope to be able to work in this > direction. If nothing else, this eliminates duplication of efforts in the long > term. Oleg, I was not expecting a switch to Sisu for this release, just thinking out loud. :) Sorry for going off-topic here. It might be interesting to open a bug to investigate a potential (long term) move of E4 DI to Sisu and discuss with the Sisu team though. Back to the main topic, my main concern for this issue is having a clean support for OSGi services with the E4 DI engine we know! Applied patch: http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=5ac37d109d815c15a42bf8b626b87870570533e1 And an extra piece: http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=dee2acec3c876b69c1522aa925f9ee85a3809f92 The original issue ("OSGi services aren't injected if they weren't registered...") should be fixed now. |
It is currently impossible to have the system inject "dynamic" OSGi services that are not known when the object (and thus its context) are created. For instance this snippet: class View { LogService logger = null; @Inject public synchronized void setLogService(@Optional LogService pLogger) { if (pLogger != null) { this.logger = pLogger; System.out.println("Logger is here"); } else if (this.logger != null && pLogger = null) { this.logger = null; System.out.println("Logger is gone"); } else { System.out.println("Logger is not there yet"); } } } This code works fine if the LogService was registered at the time View is instanciated; but if it arrives *later*, the setter will only be called with null as an argument and never with the actual LogService instance. In other words, it might print: (run with lower start level for logger) "Logger is here" (stop logger) "Logger is gone" (restart log bundle) "Logger is here" (...) or (run with higher start level for logger) "Logger is not there yet" (logger arrives) prints nothing (restart logger bundle) prints nothing.. etc This limit currently prevents the full use of the OSGi service programming model with E4's injection. After a discussion with Paul Webster, it appears the problem is that the context does not keep a reference to unresolved optional items to try to set them later (in the case of OSGi services at least). For the record, as I need the feature for a product I am currently developing, I'll try to make it work ASAP and provide a patch.