Bugzilla – Full Text Bug Listing
| Summary: | add support for metatype on declarative services | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Bryan Hunt <bhunt> | ||||||||||||||
| Component: | Compendium | Assignee: | John Ross <jwross> | ||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||
| Severity: | enhancement | ||||||||||||||||
| Priority: | P3 | CC: | simon_kaegi, tjwatson | ||||||||||||||
| Version: | unspecified | ||||||||||||||||
| Target Milestone: | 3.7 M3 | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Bryan Hunt
Created attachment 166704 [details]
improved patch
I figured out that you can have complex filters, so I removed the additional service tracker. I've also added some error handling and changed the service properties based on feedback from Tom.
Thanks Bryan, A more complex filter string does have some performance cost. The framework is able to easily optimize service filters for which it is each to index on the objectClass key. See bug 151645. // just checking for simple filters here where objectClass is // the only attr or it is one attr of a base '&' clause (objectClass=org.acme.BrickService) OK (&(objectClass=org.acme.BrickService)(|(vendor=IBM)(vendor=SUN))) OK (objectClass=org.acme.*) NOT OK (|(objectClass=org.acme.BrickService)(objectClass=org.acme.CementService)) NOT OK (&(objectClass=org.acme.BrickService)(objectClass=org.acme.CementService)) OK but only the first objectClass is returned The original filter in metatype which checks for ManagedService or | ManagedServiceFactory implementations already prevents us from optimizing it so your addition of another | clause does not make it worse. But it may be good to split it out into three filters for performance reasons. So, if I understand you correctly, it would be better to have three service trackers with the following filters: (objectClass=org.osgi.service.cm.ManagedService) (objectClass=org.osgi.service.cm.ManagedServiceFactory) (&(objectClass=org.sogi.service.metatype.MetaTypeProvider)(metatype.provider.pid=*)(metatype.provider.type=*)) (In reply to comment #3) > So, if I understand you correctly, it would be better to have three service > trackers with the following filters: > > (objectClass=org.osgi.service.cm.ManagedService) > (objectClass=org.osgi.service.cm.ManagedServiceFactory) > (&(objectClass=org.sogi.service.metatype.MetaTypeProvider)(metatype.provider.pid=*)(metatype.provider.type=*)) Yes, that is correct. Although I realize this has nothing to do with the topic of this bug. Perhaps we should open a different bug to consider restructuring the trackers for Metatype. I opened bug311716 to track any optimization. You can keep the current approach, using a single filter for this bug. Spec is moving forward with the suggested enhancement. I think we should be able to release this into 3.7. John, could you work with Bryan on this one. Bryan had proposed an enhancement to OSGi in bug https://www.osgi.org/members/bugzilla/show_bug.cgi?id=1641 The attached patch implements his original proposal. The actual solution put forth from OSGi is a bit different I think. Need to review the spec on this. (In reply to comment #6) > John, could you work with Bryan on this one. Bryan had proposed an enhancement > to OSGi in bug https://www.osgi.org/members/bugzilla/show_bug.cgi?id=1641 > The attached patch implements his original proposal. The actual solution put > forth from OSGi is a bit different I think. Need to review the spec on this. I'll go ahead and proceed with this based on https://www.osgi.org/members/bugzilla/show_bug.cgi?id=1641#c12 and https://www.osgi.org/members/bugzilla/show_bug.cgi?id=1641#c13. That seems like the right way to do it, but I suppose it could change before the spec is finalized. Created attachment 181141 [details] Updated patch based for latest HEAD contents and spec I updated the patch for the latest HEAD contents as well as to reflect the current spec. Now supports type String+ for the SERVICE_PID property. Updates the filter based on https://www.osgi.org/members/bugzilla/show_bug.cgi?id=1641#c12. Uses ServiceTracker.getTracked() instead of ServiceTracker.getServiceReferences() combined with bundleContext.getService() and ungetService(). Made the tracker static since it can be shared by all MetaTypeProviderTracker instances; the filtering by bundle is done within the instance. Implements the direction as discussed in https://www.osgi.org/members/bugzilla/show_bug.cgi?id=1641#c12 and https://www.osgi.org/members/bugzilla/show_bug.cgi?id=1641#c13. Passes a LogService to MetaTypeProviderTracker so ignored MetaTypeProviders are logged. Adds project specific support for PDE/API Tools Environment Descriptions. (not part of the patch) I also added a new CT for metatype that tests for a metatype provider registration using METATYPE_PID and that's not a managed service or managed service factory. One possible improvement would be for each MetaTypeProviderTracker to have its own ServiceTracker instance but provide a ServiceTrackerCustomizer in order to filter by bundle each time a new service matching the filter is registered. This would avoid the need to loop over the entire set of possible services with each call to every MetaTypeInformation method except getBundle() at the cost of having more ServiceTracker instances monitoring the registry. Any comments regarding the tradeoffs? - MetaTypeProviderTracker.tracker is never closed, if you stop and start the metatype impl bundle the static tracker will be invalid because the bundle context used to create it will be invalid. I think the tracker should be an instance field of MetaTypeServiceImpl and passed to the constructor of MetaTypeProviderTracker so that you can share the tracker between the instances of MetaTypeProviderTracker. It may be more simple to keep the tracker in the Activator and pass it down throw the constructors of MetaTypeServiceImpl/MetaTypeProviderTracker. - Not related to patch. The doPrivileged block in org.eclipse.equinox.metatype.Activator.registerMetaTypeService() is not necessary. The framework must use doPriv blocks when calling out to listeners, so the only things that should be on the permission stack is the framework, and tracker implementation (which both must have all permission). So there is no reason to protect the stack from the permission checks here. (In reply to comment #9) > One possible improvement would be for each MetaTypeProviderTracker to have its > own ServiceTracker instance but provide a ServiceTrackerCustomizer in order to > filter by bundle each time a new service matching the filter is registered. > This would avoid the need to loop over the entire set of possible services with > each call to every MetaTypeInformation method except getBundle() at the cost of > having more ServiceTracker instances monitoring the registry. > > Any comments regarding the tradeoffs? I am a bit worried with this approach. For one thing there is no lifecycle event to tell you when to close the tracker for each MetaTypeProviderTracker. These are handed out to clients and there is no method to release the object. This means we will continually pile up new registrations of trackers. I am also concerned about the complexity of the filter. It negates any optimizations in bug151645 (see comment 2 and comment 3). It likely is not much of a concern if we limit it to a single listener/tracker. In other words there likely is no measurable performance hit if we stick to one complex filter with one listener/tracker. I also wonder if org.osgi.service.metatype.MetaTypeInformation objects are intended to be dynamic or simply a snapshot of the currently available MetaTypeInformation. If just a snapshot then the implementation could be greatly simplified. (In reply to comment #10) > - MetaTypeProviderTracker.tracker is never closed, if you stop and start the > metatype impl bundle the static tracker will be invalid because the bundle > context used to create it will be invalid. I think the tracker should be an > instance field of MetaTypeServiceImpl and passed to the constructor of > MetaTypeProviderTracker so that you can share the tracker between the instances > of MetaTypeProviderTracker. It may be more simple to keep the tracker in the > Activator and pass it down throw the constructors of > MetaTypeServiceImpl/MetaTypeProviderTracker. > - Not related to patch. The doPrivileged block in > org.eclipse.equinox.metatype.Activator.registerMetaTypeService() is not > necessary. The framework must use doPriv blocks when calling out to listeners, > so the only things that should be on the permission stack is the framework, and > tracker implementation (which both must have all permission). So there is no > reason to protect the stack from the permission checks here. Okay, I'll take care of it. (In reply to comment #11) > I also wonder if org.osgi.service.metatype.MetaTypeInformation objects are > intended to be dynamic or simply a snapshot of the currently available > MetaTypeInformation. If just a snapshot then the implementation could be > greatly simplified. For bundles that register their own MetaTypeProvider (i.e. there is no metatype data under OSGI-INF/metatype), the returned MetaTypeInformation is the MetaTypeProviderTracker underneath of which is the ServiceTracker. So in that sense it would be dynamic, if that's what you meant. (In reply to comment #12) > (In reply to comment #11) > > I also wonder if org.osgi.service.metatype.MetaTypeInformation objects are > > intended to be dynamic or simply a snapshot of the currently available > > MetaTypeInformation. If just a snapshot then the implementation could be > > greatly simplified. > > For bundles that register their own MetaTypeProvider (i.e. there is no metatype > data under OSGI-INF/metatype), the returned MetaTypeInformation is the > MetaTypeProviderTracker underneath of which is the ServiceTracker. So in that > sense it would be dynamic, if that's what you meant. This makes sense, but I wonder if the spec requires this dynamic tracking. Is the MetaTypeInformation object supposed to be a long lived object or simply a snapshot that is supposed to be used to extract some data and then thrown away? I guess the point is not really valid since we likely cannot change the object's behavior to all of a sudden not be dynamic. In section 105.5 the spec does state this: getMetaTypeInformation(Bundle) – Given a bundle, it must return the Meta Type Information for that bundle, even if there is no meta type information available at the moment of the call. This seems to imply the returned object is dynamic and will reflect meta type information that becomes available after the call. Created attachment 181342 [details] Updated patch for comment 10 Removes the doPrivileged block in org.eclipse.equinox.metatype.Activator.registerMetaTypeService(). The MetaTypeProvider tracker is now kept in the Activator and passed down to the MetaTypeServiceImpl/MetaTypeProviderTracker constructors. As part of these changes, I took a stab at refactoring the Activator to address what I believed to be a number of potential concurrency issues. Let me know if you see any problems with this. We should probably have a look at the other classes as well. Created attachment 181343 [details] Updated patch for comment 10 with NPE fix Whoops. Minor change to fix potential NPE. Created attachment 181461 [details] Updated patch for 10/21 CPEG call Updates based on 10/21 CPEG call and https://www.osgi.org/members/bugzilla/show_bug.cgi?id=1641#c15. Patch released. Thanks John! |