[FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks

[glimmerveen]

This PR contains the patch already attached to FELIX-6161, but now applied on the current state of the felix-dev repository.

[tjwatson]

I have some requested changes. I also would like to see some tests that verify that the listener hooks now have the expected information.

[glimmerveen]

I have some requested changes. I also would like to see some tests that verify that the listener hooks now have the expected information.

Thanks for your feedback, I will also work on adding some tests to verify the behaviour.

[djencks]

Trying to remember the distant past…. I think that the problem this whole thing was designed to solve was that it’s possible to have several references to the same service, even from the same component. (I think this also solves it for multiple components with the same dependency, in the same bundle). If you register several listeners, then you get several events and unfortunate things can happen in between those events. My code was an attempt to get only one event per dependency, no matter how many references that is spread over. IIRC Arnoud’s change change the mapping from “all dependencies on some service”, where DS manages the filters, to “all dependencies with a certain filter on some service”, letting the service registry manage the filters. This introduces some circumstances where multiple events might be needed, but still eliminates the most likely, where for instance you get the service in one place and the service property map in another. I think my idea was to, for instance, with a static reference, get all the references to a particular service updated in between the deactivation and reactivation. With entirely separate events there’s going to be a peculiar short lived state where the two references to the same service/filter will be referring to different instances. David Jencks

…

On Apr 21, 2020, at 12:42 PM, Thomas Watson ***@***.***> wrote: @tjwatson commented on this pull request. In scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java <#19 (comment)>: > if ( listenerInfo != null ) { - if ( listenerInfo.remove( filter, listener ) ) - { - listenerMap.remove( className ); - m_context.removeServiceListener( listenerInfo ); - } + listenerMap.remove( serviceFilterString ); Part of me that is concerned about why the code was the way it is currently. I have to admit that I don't know the history of why though. For example, I'm unsure what condition causes the ListenerInfo to even have multiple ExtendedServiceListeners in its list of listeners. There appears to be a one-to-one mapping between a ServiceTracker and a DependencyManager and a DependencyManager manages a single reference and hence a single target filter for the reference. I guess I don't see how multiple targets would come from a single DependencyManager. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#19 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAELDXS67V7O4CMRS4GGWPDRNXZLHANCNFSM4MNG3OYA>.

[glimmerveen]

I have updated the PR to include an integration test as well that validates two aspects of the use of ServiceListener discussed:

1. Target filter properties visible to ListenerHook
2. If the same service is referenced multiple times from the same bundle, only a single ServiceListener is opened and maintained.

[tjwatson]

I think we are making good progress. Just a few more suggestions. I also need to run this through the tests in OpenLiberty. I ran the late version of the PR through an OpenLiberty build and it had many test failures that I hope were caused by the early removal of the listener.

[tjwatson]

Looks good to me. Thanks for the persistent work on this Arnoud.

[cziegeler]

Lgtm, thanks