You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@felix.apache.org by GitBox <gi...@apache.org> on 2020/04/21 11:13:09 UTC
[GitHub] [felix-dev] glimmerveen opened a new pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
glimmerveen opened a new pull request #19:
URL: https://github.com/apache/felix-dev/pull/19
This PR contains the patch already attached to PISTE-6161, but now applied on the current state of the felix-dev repository.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] tjwatson commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
tjwatson commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r412404198
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java
##########
@@ -98,133 +98,82 @@
private static class ListenerInfo implements ServiceListener
{
- private Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap = new HashMap<>();
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = new ArrayList<>();
@Override
public void serviceChanged(ServiceEvent event)
{
ServiceReference<?> ref = event.getServiceReference();
- ExtendedServiceEvent extEvent = null;
- ExtendedServiceEvent endMatchEvent = null;
- Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap;
+ ExtendedServiceEvent extEvent = new ExtendedServiceEvent(event);
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners;
synchronized ( this )
{
- filterMap = this.filterMap;
+ listeners = this.listeners;
}
- for ( Map.Entry<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> entry : filterMap.entrySet() )
+
+ for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : listeners)
{
- Filter filter = entry.getKey();
- if ( filter == null || filter.match( ref ) )
- {
- if ( extEvent == null )
- {
- extEvent = new ExtendedServiceEvent( event );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( extEvent );
- }
- }
- else if ( event.getType() == ServiceEvent.MODIFIED )
- {
- if ( endMatchEvent == null )
- {
- endMatchEvent = new ExtendedServiceEvent( ServiceEvent.MODIFIED_ENDMATCH, ref );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( endMatchEvent );
- }
- }
+ forwardTo.serviceChanged( extEvent );
}
+
if ( extEvent != null )
{
extEvent.activateManagers();
}
- if ( endMatchEvent != null )
- {
- endMatchEvent.activateManagers();
- }
}
- public synchronized void add(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized void add(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- filterMap = new HashMap<>( filterMap );
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners == null )
- {
- listeners = Collections.<ExtendedServiceListener<ExtendedServiceEvent>> singletonList( listener );
- }
- else
- {
- listeners = new ArrayList<>( listeners );
- listeners.add( listener );
- }
- filterMap.put( filter, listeners );
+ listeners = new ArrayList<>(listeners);
+ listeners.add(listener);
}
- public synchronized boolean remove(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized boolean remove(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners != null )
- {
- filterMap = new HashMap<>( filterMap );
- listeners = new ArrayList<>( listeners );
- listeners.remove( listener );
- if ( listeners.isEmpty() )
- {
- filterMap.remove( filter );
- }
- else
- {
- filterMap.put( filter, listeners );
- }
- }
- return filterMap.isEmpty();
+ listeners = new ArrayList<>(listeners);
+ listeners.remove(listener);
+ return listeners.isEmpty();
}
}
@Override
- public void addServiceListener(String classNameFilter, Filter eventFilter,
+ public void addServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
ListenerInfo listenerInfo;
synchronized ( listenerMap )
{
- logger.log( LogService.LOG_DEBUG, "classNameFilter: " + classNameFilter + " event filter: " + eventFilter, null);
- listenerInfo = listenerMap.get( classNameFilter );
+ logger.log( LogService.LOG_DEBUG, "serviceFilterString: " + serviceFilterString, null);
+ listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo == null )
{
listenerInfo = new ListenerInfo();
- listenerMap.put( classNameFilter, listenerInfo );
+ listenerMap.put( serviceFilterString, listenerInfo );
try
{
- m_context.addServiceListener( listenerInfo, classNameFilter );
+ m_context.addServiceListener( listenerInfo, serviceFilterString );
}
catch ( InvalidSyntaxException e )
{
throw (IllegalArgumentException) new IllegalArgumentException(
"invalid class name filter" ).initCause( e );
}
}
+ listenerInfo.add(listener);
}
- listenerInfo.add( eventFilter, listener );
}
@Override
- public void removeServiceListener(String className, Filter filter,
+ public void removeServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
synchronized ( listenerMap )
{
- ListenerInfo listenerInfo = listenerMap.get( className );
+ ListenerInfo listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo != null )
{
- if ( listenerInfo.remove( filter, listener ) )
- {
- listenerMap.remove( className );
- m_context.removeServiceListener( listenerInfo );
- }
+ listenerMap.remove( serviceFilterString );
Review comment:
This always removes the the ListenerInfo even if there are others still in the ListenerInfo?
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
##########
@@ -2283,8 +2283,8 @@ else if ( m_dependencyMetadata.getScope() == ReferenceScope.prototype_required )
null, getName(), initialActive, refMap, classFilterString, eventFilter,
initialReferenceFilterString );
ServiceTracker<T, RefPair<S, T>, ExtendedServiceEvent> tracker = new ServiceTracker<>(
- bundleContext, m_customizer, initialActive, m_componentManager.getActivator(), eventFilter,
Review comment:
The use of eventFilter was removed but the Filter constructed from the eventFilterString is still done above. This doesn't seem necessary anymore.
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java
##########
@@ -98,133 +98,82 @@
private static class ListenerInfo implements ServiceListener
{
- private Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap = new HashMap<>();
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = new ArrayList<>();
Review comment:
Shouldn't this just be a CopyOnWriteArrayList?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] tjwatson commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
tjwatson commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r414049078
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
##########
@@ -2239,52 +2238,16 @@ else if ( m_dependencyMetadata.getScope() == ReferenceScope.prototype_required )
m_componentManager.getLogger().log(LogService.LOG_DEBUG, "Setting target property for dependency {0} to {1}",
null, getName(), target );
BundleContext bundleContext = m_componentManager.getBundleContext();
- Filter eventFilter = null;
Review comment:
Good idea, looks good.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] glimmerveen commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
glimmerveen commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r412716542
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java
##########
@@ -98,133 +98,82 @@
private static class ListenerInfo implements ServiceListener
{
- private Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap = new HashMap<>();
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = new ArrayList<>();
@Override
public void serviceChanged(ServiceEvent event)
{
ServiceReference<?> ref = event.getServiceReference();
- ExtendedServiceEvent extEvent = null;
- ExtendedServiceEvent endMatchEvent = null;
- Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap;
+ ExtendedServiceEvent extEvent = new ExtendedServiceEvent(event);
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners;
synchronized ( this )
{
- filterMap = this.filterMap;
+ listeners = this.listeners;
}
- for ( Map.Entry<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> entry : filterMap.entrySet() )
+
+ for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : listeners)
{
- Filter filter = entry.getKey();
- if ( filter == null || filter.match( ref ) )
- {
- if ( extEvent == null )
- {
- extEvent = new ExtendedServiceEvent( event );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( extEvent );
- }
- }
- else if ( event.getType() == ServiceEvent.MODIFIED )
- {
- if ( endMatchEvent == null )
- {
- endMatchEvent = new ExtendedServiceEvent( ServiceEvent.MODIFIED_ENDMATCH, ref );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( endMatchEvent );
- }
- }
+ forwardTo.serviceChanged( extEvent );
}
+
if ( extEvent != null )
{
extEvent.activateManagers();
}
- if ( endMatchEvent != null )
- {
- endMatchEvent.activateManagers();
- }
}
- public synchronized void add(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized void add(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- filterMap = new HashMap<>( filterMap );
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners == null )
- {
- listeners = Collections.<ExtendedServiceListener<ExtendedServiceEvent>> singletonList( listener );
- }
- else
- {
- listeners = new ArrayList<>( listeners );
- listeners.add( listener );
- }
- filterMap.put( filter, listeners );
+ listeners = new ArrayList<>(listeners);
+ listeners.add(listener);
}
- public synchronized boolean remove(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized boolean remove(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners != null )
- {
- filterMap = new HashMap<>( filterMap );
- listeners = new ArrayList<>( listeners );
- listeners.remove( listener );
- if ( listeners.isEmpty() )
- {
- filterMap.remove( filter );
- }
- else
- {
- filterMap.put( filter, listeners );
- }
- }
- return filterMap.isEmpty();
+ listeners = new ArrayList<>(listeners);
+ listeners.remove(listener);
+ return listeners.isEmpty();
}
}
@Override
- public void addServiceListener(String classNameFilter, Filter eventFilter,
+ public void addServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
ListenerInfo listenerInfo;
synchronized ( listenerMap )
{
- logger.log( LogService.LOG_DEBUG, "classNameFilter: " + classNameFilter + " event filter: " + eventFilter, null);
- listenerInfo = listenerMap.get( classNameFilter );
+ logger.log( LogService.LOG_DEBUG, "serviceFilterString: " + serviceFilterString, null);
+ listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo == null )
{
listenerInfo = new ListenerInfo();
- listenerMap.put( classNameFilter, listenerInfo );
+ listenerMap.put( serviceFilterString, listenerInfo );
try
{
- m_context.addServiceListener( listenerInfo, classNameFilter );
+ m_context.addServiceListener( listenerInfo, serviceFilterString );
}
catch ( InvalidSyntaxException e )
{
throw (IllegalArgumentException) new IllegalArgumentException(
"invalid class name filter" ).initCause( e );
}
}
+ listenerInfo.add(listener);
}
- listenerInfo.add( eventFilter, listener );
}
@Override
- public void removeServiceListener(String className, Filter filter,
+ public void removeServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
synchronized ( listenerMap )
{
- ListenerInfo listenerInfo = listenerMap.get( className );
+ ListenerInfo listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo != null )
{
- if ( listenerInfo.remove( filter, listener ) )
- {
- listenerMap.remove( className );
- m_context.removeServiceListener( listenerInfo );
- }
+ listenerMap.remove( serviceFilterString );
Review comment:
@djencks I believe my current changes reflect the behaviour you suggested, and I have added a test to verify this.
The important change also here that supports my use-case is the fact that the target filter is now also visible to the ListenerHook's, allowing such an implementation to register a service that will match the target filter.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] glimmerveen commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
glimmerveen commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r412428375
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java
##########
@@ -98,133 +98,82 @@
private static class ListenerInfo implements ServiceListener
{
- private Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap = new HashMap<>();
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = new ArrayList<>();
Review comment:
It has been a while I made these changes, so I am not too sure, but I believe I mimicked the current approach, but with a List rather than a Map. Looking at the places where the list is modified, it looks similar to what a CopyOnWriteArrayList would do.
I don't mind changing it, though I am not familiar with possible additional reasons why it was done in this way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] tjwatson commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
tjwatson commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r413033652
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java
##########
@@ -98,133 +98,82 @@
private static class ListenerInfo implements ServiceListener
{
- private Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap = new HashMap<>();
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = new ArrayList<>();
@Override
public void serviceChanged(ServiceEvent event)
{
ServiceReference<?> ref = event.getServiceReference();
- ExtendedServiceEvent extEvent = null;
- ExtendedServiceEvent endMatchEvent = null;
- Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap;
+ ExtendedServiceEvent extEvent = new ExtendedServiceEvent(event);
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners;
synchronized ( this )
{
- filterMap = this.filterMap;
+ listeners = this.listeners;
}
- for ( Map.Entry<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> entry : filterMap.entrySet() )
+
+ for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : listeners)
{
- Filter filter = entry.getKey();
- if ( filter == null || filter.match( ref ) )
- {
- if ( extEvent == null )
- {
- extEvent = new ExtendedServiceEvent( event );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( extEvent );
- }
- }
- else if ( event.getType() == ServiceEvent.MODIFIED )
- {
- if ( endMatchEvent == null )
- {
- endMatchEvent = new ExtendedServiceEvent( ServiceEvent.MODIFIED_ENDMATCH, ref );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( endMatchEvent );
- }
- }
+ forwardTo.serviceChanged( extEvent );
}
+
if ( extEvent != null )
{
extEvent.activateManagers();
}
- if ( endMatchEvent != null )
- {
- endMatchEvent.activateManagers();
- }
}
- public synchronized void add(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized void add(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- filterMap = new HashMap<>( filterMap );
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners == null )
- {
- listeners = Collections.<ExtendedServiceListener<ExtendedServiceEvent>> singletonList( listener );
- }
- else
- {
- listeners = new ArrayList<>( listeners );
- listeners.add( listener );
- }
- filterMap.put( filter, listeners );
+ listeners = new ArrayList<>(listeners);
+ listeners.add(listener);
}
- public synchronized boolean remove(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized boolean remove(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners != null )
- {
- filterMap = new HashMap<>( filterMap );
- listeners = new ArrayList<>( listeners );
- listeners.remove( listener );
- if ( listeners.isEmpty() )
- {
- filterMap.remove( filter );
- }
- else
- {
- filterMap.put( filter, listeners );
- }
- }
- return filterMap.isEmpty();
+ listeners = new ArrayList<>(listeners);
+ listeners.remove(listener);
+ return listeners.isEmpty();
}
}
@Override
- public void addServiceListener(String classNameFilter, Filter eventFilter,
+ public void addServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
ListenerInfo listenerInfo;
synchronized ( listenerMap )
{
- logger.log( LogService.LOG_DEBUG, "classNameFilter: " + classNameFilter + " event filter: " + eventFilter, null);
- listenerInfo = listenerMap.get( classNameFilter );
+ logger.log( LogService.LOG_DEBUG, "serviceFilterString: " + serviceFilterString, null);
+ listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo == null )
{
listenerInfo = new ListenerInfo();
- listenerMap.put( classNameFilter, listenerInfo );
+ listenerMap.put( serviceFilterString, listenerInfo );
try
{
- m_context.addServiceListener( listenerInfo, classNameFilter );
+ m_context.addServiceListener( listenerInfo, serviceFilterString );
}
catch ( InvalidSyntaxException e )
{
throw (IllegalArgumentException) new IllegalArgumentException(
"invalid class name filter" ).initCause( e );
}
}
+ listenerInfo.add(listener);
}
- listenerInfo.add( eventFilter, listener );
}
@Override
- public void removeServiceListener(String className, Filter filter,
+ public void removeServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
synchronized ( listenerMap )
{
- ListenerInfo listenerInfo = listenerMap.get( className );
+ ListenerInfo listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo != null )
{
- if ( listenerInfo.remove( filter, listener ) )
- {
- listenerMap.remove( className );
- m_context.removeServiceListener( listenerInfo );
- }
+ listenerMap.remove( serviceFilterString );
Review comment:
OK, I have a better understanding of how the listeners are managed now! I was getting stuck on the fact that we have a ServiceTracker created per reference, even when there are multiple references from the same bundle with the idententical target. I was missing the fact that the ServiceTracker uses the `ExtendedServiceListenerContext` to manage the registration of the listener and that there is a single `ExtendedServiceListenerContext` per bundle.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] glimmerveen commented on issue #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
glimmerveen commented on issue #19:
URL: https://github.com/apache/felix-dev/pull/19#issuecomment-617373529
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] glimmerveen commented on issue #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
glimmerveen commented on issue #19:
URL: https://github.com/apache/felix-dev/pull/19#issuecomment-617584505
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] djencks commented on issue #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
djencks commented on issue #19:
URL: https://github.com/apache/felix-dev/pull/19#issuecomment-617384334
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 <no...@github.com> wrote:
>
>
> @tjwatson commented on this pull request.
>
> In scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java <https://github.com/apache/felix-dev/pull/19#discussion_r412440777>:
>
> > 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 <https://github.com/apache/felix-dev/pull/19#discussion_r412440777>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAELDXS67V7O4CMRS4GGWPDRNXZLHANCNFSM4MNG3OYA>.
>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] djencks commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
djencks commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r412465282
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java
##########
@@ -98,133 +98,82 @@
private static class ListenerInfo implements ServiceListener
{
- private Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap = new HashMap<>();
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = new ArrayList<>();
@Override
public void serviceChanged(ServiceEvent event)
{
ServiceReference<?> ref = event.getServiceReference();
- ExtendedServiceEvent extEvent = null;
- ExtendedServiceEvent endMatchEvent = null;
- Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap;
+ ExtendedServiceEvent extEvent = new ExtendedServiceEvent(event);
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners;
synchronized ( this )
{
- filterMap = this.filterMap;
+ listeners = this.listeners;
}
- for ( Map.Entry<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> entry : filterMap.entrySet() )
+
+ for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : listeners)
{
- Filter filter = entry.getKey();
- if ( filter == null || filter.match( ref ) )
- {
- if ( extEvent == null )
- {
- extEvent = new ExtendedServiceEvent( event );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( extEvent );
- }
- }
- else if ( event.getType() == ServiceEvent.MODIFIED )
- {
- if ( endMatchEvent == null )
- {
- endMatchEvent = new ExtendedServiceEvent( ServiceEvent.MODIFIED_ENDMATCH, ref );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( endMatchEvent );
- }
- }
+ forwardTo.serviceChanged( extEvent );
}
+
if ( extEvent != null )
{
extEvent.activateManagers();
}
- if ( endMatchEvent != null )
- {
- endMatchEvent.activateManagers();
- }
}
- public synchronized void add(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized void add(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- filterMap = new HashMap<>( filterMap );
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners == null )
- {
- listeners = Collections.<ExtendedServiceListener<ExtendedServiceEvent>> singletonList( listener );
- }
- else
- {
- listeners = new ArrayList<>( listeners );
- listeners.add( listener );
- }
- filterMap.put( filter, listeners );
+ listeners = new ArrayList<>(listeners);
+ listeners.add(listener);
}
- public synchronized boolean remove(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized boolean remove(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners != null )
- {
- filterMap = new HashMap<>( filterMap );
- listeners = new ArrayList<>( listeners );
- listeners.remove( listener );
- if ( listeners.isEmpty() )
- {
- filterMap.remove( filter );
- }
- else
- {
- filterMap.put( filter, listeners );
- }
- }
- return filterMap.isEmpty();
+ listeners = new ArrayList<>(listeners);
+ listeners.remove(listener);
+ return listeners.isEmpty();
}
}
@Override
- public void addServiceListener(String classNameFilter, Filter eventFilter,
+ public void addServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
ListenerInfo listenerInfo;
synchronized ( listenerMap )
{
- logger.log( LogService.LOG_DEBUG, "classNameFilter: " + classNameFilter + " event filter: " + eventFilter, null);
- listenerInfo = listenerMap.get( classNameFilter );
+ logger.log( LogService.LOG_DEBUG, "serviceFilterString: " + serviceFilterString, null);
+ listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo == null )
{
listenerInfo = new ListenerInfo();
- listenerMap.put( classNameFilter, listenerInfo );
+ listenerMap.put( serviceFilterString, listenerInfo );
try
{
- m_context.addServiceListener( listenerInfo, classNameFilter );
+ m_context.addServiceListener( listenerInfo, serviceFilterString );
}
catch ( InvalidSyntaxException e )
{
throw (IllegalArgumentException) new IllegalArgumentException(
"invalid class name filter" ).initCause( e );
}
}
+ listenerInfo.add(listener);
}
- listenerInfo.add( eventFilter, listener );
}
@Override
- public void removeServiceListener(String className, Filter filter,
+ public void removeServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
synchronized ( listenerMap )
{
- ListenerInfo listenerInfo = listenerMap.get( className );
+ ListenerInfo listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo != null )
{
- if ( listenerInfo.remove( filter, listener ) )
- {
- listenerMap.remove( className );
- m_context.removeServiceListener( listenerInfo );
- }
+ listenerMap.remove( serviceFilterString );
Review comment:
That is my recollection of how it worked. I'd recommend in this change aggregating all the references to a service with a particular filter from a particular bundle to get the minimum number of events. Is that consistent with the "service on request" you are trying to implement?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] glimmerveen commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
glimmerveen commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r412444967
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java
##########
@@ -98,133 +98,82 @@
private static class ListenerInfo implements ServiceListener
{
- private Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap = new HashMap<>();
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = new ArrayList<>();
@Override
public void serviceChanged(ServiceEvent event)
{
ServiceReference<?> ref = event.getServiceReference();
- ExtendedServiceEvent extEvent = null;
- ExtendedServiceEvent endMatchEvent = null;
- Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap;
+ ExtendedServiceEvent extEvent = new ExtendedServiceEvent(event);
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners;
synchronized ( this )
{
- filterMap = this.filterMap;
+ listeners = this.listeners;
}
- for ( Map.Entry<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> entry : filterMap.entrySet() )
+
+ for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : listeners)
{
- Filter filter = entry.getKey();
- if ( filter == null || filter.match( ref ) )
- {
- if ( extEvent == null )
- {
- extEvent = new ExtendedServiceEvent( event );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( extEvent );
- }
- }
- else if ( event.getType() == ServiceEvent.MODIFIED )
- {
- if ( endMatchEvent == null )
- {
- endMatchEvent = new ExtendedServiceEvent( ServiceEvent.MODIFIED_ENDMATCH, ref );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( endMatchEvent );
- }
- }
+ forwardTo.serviceChanged( extEvent );
}
+
if ( extEvent != null )
{
extEvent.activateManagers();
}
- if ( endMatchEvent != null )
- {
- endMatchEvent.activateManagers();
- }
}
- public synchronized void add(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized void add(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- filterMap = new HashMap<>( filterMap );
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners == null )
- {
- listeners = Collections.<ExtendedServiceListener<ExtendedServiceEvent>> singletonList( listener );
- }
- else
- {
- listeners = new ArrayList<>( listeners );
- listeners.add( listener );
- }
- filterMap.put( filter, listeners );
+ listeners = new ArrayList<>(listeners);
+ listeners.add(listener);
}
- public synchronized boolean remove(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized boolean remove(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners != null )
- {
- filterMap = new HashMap<>( filterMap );
- listeners = new ArrayList<>( listeners );
- listeners.remove( listener );
- if ( listeners.isEmpty() )
- {
- filterMap.remove( filter );
- }
- else
- {
- filterMap.put( filter, listeners );
- }
- }
- return filterMap.isEmpty();
+ listeners = new ArrayList<>(listeners);
+ listeners.remove(listener);
+ return listeners.isEmpty();
}
}
@Override
- public void addServiceListener(String classNameFilter, Filter eventFilter,
+ public void addServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
ListenerInfo listenerInfo;
synchronized ( listenerMap )
{
- logger.log( LogService.LOG_DEBUG, "classNameFilter: " + classNameFilter + " event filter: " + eventFilter, null);
- listenerInfo = listenerMap.get( classNameFilter );
+ logger.log( LogService.LOG_DEBUG, "serviceFilterString: " + serviceFilterString, null);
+ listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo == null )
{
listenerInfo = new ListenerInfo();
- listenerMap.put( classNameFilter, listenerInfo );
+ listenerMap.put( serviceFilterString, listenerInfo );
try
{
- m_context.addServiceListener( listenerInfo, classNameFilter );
+ m_context.addServiceListener( listenerInfo, serviceFilterString );
}
catch ( InvalidSyntaxException e )
{
throw (IllegalArgumentException) new IllegalArgumentException(
"invalid class name filter" ).initCause( e );
}
}
+ listenerInfo.add(listener);
}
- listenerInfo.add( eventFilter, listener );
}
@Override
- public void removeServiceListener(String className, Filter filter,
+ public void removeServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
synchronized ( listenerMap )
{
- ListenerInfo listenerInfo = listenerMap.get( className );
+ ListenerInfo listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo != null )
{
- if ( listenerInfo.remove( filter, listener ) )
- {
- listenerMap.remove( className );
- m_context.removeServiceListener( listenerInfo );
- }
+ listenerMap.remove( serviceFilterString );
Review comment:
Perhaps it is related to current approach of effectively aggregating multiple references into a single ServiceListener. As this PR aims to remove this aggregation, perhaps this is something that can be further simplified?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] tjwatson commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
tjwatson commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r413042421
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/manager/ServiceTracker.java
##########
@@ -76,13 +76,6 @@
* The Bundle Context used by this {@code ServiceTracker}.
*/
protected final BundleContext context;
- /**
- * The Filter used by this {@code ServiceTracker} which specifies the search
- * criteria for the services to track.
- *
- * @since 1.1
- */
- protected final Filter eventFilter;
Review comment:
Leaves an unused import above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] tjwatson commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
tjwatson commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r413037777
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/manager/ExtendedServiceListenerContext.java
##########
@@ -24,9 +24,9 @@
public interface ExtendedServiceListenerContext<U extends ServiceEvent>
{
- void addServiceListener(String className, Filter filter,
Review comment:
The removal of the Filter parameter leaves an unused import above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] glimmerveen commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
glimmerveen commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r413552250
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
##########
@@ -2239,52 +2238,16 @@ else if ( m_dependencyMetadata.getScope() == ReferenceScope.prototype_required )
m_componentManager.getLogger().log(LogService.LOG_DEBUG, "Setting target property for dependency {0} to {1}",
null, getName(), target );
BundleContext bundleContext = m_componentManager.getBundleContext();
- Filter eventFilter = null;
Review comment:
I implemented this, but opted to use a different target in case of an invalid one in the component definition, that is guaranteed to be never be satisfied (inspiration drawn from the required capability in the osgi.cmpn API bundle).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] tjwatson commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
tjwatson commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r413000181
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java
##########
@@ -98,132 +98,83 @@
private static class ListenerInfo implements ServiceListener
{
- private Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap = new HashMap<>();
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = new ArrayList<>();
@Override
public void serviceChanged(ServiceEvent event)
{
ServiceReference<?> ref = event.getServiceReference();
Review comment:
The `ref` variable is no longer used.
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
##########
@@ -2239,52 +2238,16 @@ else if ( m_dependencyMetadata.getScope() == ReferenceScope.prototype_required )
m_componentManager.getLogger().log(LogService.LOG_DEBUG, "Setting target property for dependency {0} to {1}",
null, getName(), target );
BundleContext bundleContext = m_componentManager.getBundleContext();
Review comment:
There is an additional block above that is no longer needed also:
```
// eventFilter
String eventFilterString;
if (m_target != null
&& m_dependencyMetadata.getScope() == ReferenceScope.prototype_required)
{
final StringBuilder sb = new StringBuilder("(&").append(PROTOTYPE_SCOPE_CLAUSE).append(m_target).append(")");
eventFilterString = sb.toString();
}
else if ( m_dependencyMetadata.getScope() == ReferenceScope.prototype_required )
{
eventFilterString = PROTOTYPE_SCOPE_CLAUSE;
}
else
{
eventFilterString = m_target;
}
```
##########
File path: scr/src/test/resources/integration_test_FELIX_6161.xml
##########
@@ -0,0 +1,64 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+<components>
+ <scr:component name="felix.6161"
+ xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0"
+ immediate="true"
+ enabled="false"
+ configuration-policy="optional"
+ activate="activate"
+ modified="modified">
+ <implementation class="org.apache.felix.scr.integration.components.SimpleComponent" />
+ <service>
+ <provide interface="org.apache.felix.scr.integration.components.SimpleComponent" />
+ </service>
+ <reference
+ name="one"
+ interface="org.apache.felix.scr.integration.components.SimpleService"
+ cardinality="0..1"
+ policy="dynamic"
+ bind="setSimpleService"
+ unbind="unsetSimpleService"
+ target="(value=foo)"
+ />
+ </scr:component>
+
+ <scr:component name="felix.6161.2nd"
+ xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0"
+ immediate="true"
+ enabled="false"
+ configuration-policy="optional"
+ activate="activate"
+ modified="modified">
+ <implementation class="org.apache.felix.scr.integration.components.SimpleComponent" />
+ <service>
+ <provide interface="org.apache.felix.scr.integration.components.SimpleComponent" />
+ </service>
+ <reference
+ name="one"
+ interface="org.apache.felix.scr.integration.components.SimpleService"
+ cardinality="0..1"
+ policy="dynamic"
+ bind="setSimpleService"
+ unbind="unsetSimpleService"
+ target="(value=foo)"
+ />
+ </scr:component>
Review comment:
I suggest a third component that uses a different target filter from the other two. And a forth that has an invalid filter for target.
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
##########
@@ -2239,52 +2238,16 @@ else if ( m_dependencyMetadata.getScope() == ReferenceScope.prototype_required )
m_componentManager.getLogger().log(LogService.LOG_DEBUG, "Setting target property for dependency {0} to {1}",
null, getName(), target );
BundleContext bundleContext = m_componentManager.getBundleContext();
- Filter eventFilter = null;
Review comment:
I tried to find if there is a specified behavior in the DS spec if the target has an invalid filter. I could not find anything. Regardless, the current Felix SCR impl seems to log an error here and then create this target `(component.id=-1)` filter to be used instead.
I suggest we still need this check with something like this:
```
if (target != null)
{
try
{
FrameworkUtil.createFilter(target);
}
catch (InvalidSyntaxException e)
{
m_componentManager.getLogger().log(LogService.LOG_ERROR,
"Invalid syntax in target property for dependency {0} to {1}", null,
getName(), target);
//create a filter that will never be satisfied
target = "(component.id=-1)";
}
}
m_target = target;
```
Where we do the check on the incoming `target` param before setting it to the instance `m_target`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] tjwatson commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
tjwatson commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r413043524
##########
File path: scr/src/test/java/org/apache/felix/scr/impl/manager/SingleComponentManagerTest.java
##########
@@ -51,15 +51,15 @@
private ComponentActivator componentActivator = new ComponentActivator() {
@Override
- public void addServiceListener(String className, Filter filter,
Review comment:
Causes unused import
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] tjwatson commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
tjwatson commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r412999180
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java
##########
@@ -98,133 +98,82 @@
private static class ListenerInfo implements ServiceListener
{
- private Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap = new HashMap<>();
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = new ArrayList<>();
Review comment:
I understand. I think I will make this change after this PR is merged. If we change to a CopyOnWriteList then we can get rid of the synch blocks around access to the listener snapshot.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] glimmerveen commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
glimmerveen commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r412434960
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java
##########
@@ -98,133 +98,82 @@
private static class ListenerInfo implements ServiceListener
{
- private Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap = new HashMap<>();
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = new ArrayList<>();
@Override
public void serviceChanged(ServiceEvent event)
{
ServiceReference<?> ref = event.getServiceReference();
- ExtendedServiceEvent extEvent = null;
- ExtendedServiceEvent endMatchEvent = null;
- Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap;
+ ExtendedServiceEvent extEvent = new ExtendedServiceEvent(event);
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners;
synchronized ( this )
{
- filterMap = this.filterMap;
+ listeners = this.listeners;
}
- for ( Map.Entry<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> entry : filterMap.entrySet() )
+
+ for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : listeners)
{
- Filter filter = entry.getKey();
- if ( filter == null || filter.match( ref ) )
- {
- if ( extEvent == null )
- {
- extEvent = new ExtendedServiceEvent( event );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( extEvent );
- }
- }
- else if ( event.getType() == ServiceEvent.MODIFIED )
- {
- if ( endMatchEvent == null )
- {
- endMatchEvent = new ExtendedServiceEvent( ServiceEvent.MODIFIED_ENDMATCH, ref );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( endMatchEvent );
- }
- }
+ forwardTo.serviceChanged( extEvent );
}
+
if ( extEvent != null )
{
extEvent.activateManagers();
}
- if ( endMatchEvent != null )
- {
- endMatchEvent.activateManagers();
- }
}
- public synchronized void add(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized void add(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- filterMap = new HashMap<>( filterMap );
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners == null )
- {
- listeners = Collections.<ExtendedServiceListener<ExtendedServiceEvent>> singletonList( listener );
- }
- else
- {
- listeners = new ArrayList<>( listeners );
- listeners.add( listener );
- }
- filterMap.put( filter, listeners );
+ listeners = new ArrayList<>(listeners);
+ listeners.add(listener);
}
- public synchronized boolean remove(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized boolean remove(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners != null )
- {
- filterMap = new HashMap<>( filterMap );
- listeners = new ArrayList<>( listeners );
- listeners.remove( listener );
- if ( listeners.isEmpty() )
- {
- filterMap.remove( filter );
- }
- else
- {
- filterMap.put( filter, listeners );
- }
- }
- return filterMap.isEmpty();
+ listeners = new ArrayList<>(listeners);
+ listeners.remove(listener);
+ return listeners.isEmpty();
}
}
@Override
- public void addServiceListener(String classNameFilter, Filter eventFilter,
+ public void addServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
ListenerInfo listenerInfo;
synchronized ( listenerMap )
{
- logger.log( LogService.LOG_DEBUG, "classNameFilter: " + classNameFilter + " event filter: " + eventFilter, null);
- listenerInfo = listenerMap.get( classNameFilter );
+ logger.log( LogService.LOG_DEBUG, "serviceFilterString: " + serviceFilterString, null);
+ listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo == null )
{
listenerInfo = new ListenerInfo();
- listenerMap.put( classNameFilter, listenerInfo );
+ listenerMap.put( serviceFilterString, listenerInfo );
try
{
- m_context.addServiceListener( listenerInfo, classNameFilter );
+ m_context.addServiceListener( listenerInfo, serviceFilterString );
}
catch ( InvalidSyntaxException e )
{
throw (IllegalArgumentException) new IllegalArgumentException(
"invalid class name filter" ).initCause( e );
}
}
+ listenerInfo.add(listener);
}
- listenerInfo.add( eventFilter, listener );
}
@Override
- public void removeServiceListener(String className, Filter filter,
+ public void removeServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
synchronized ( listenerMap )
{
- ListenerInfo listenerInfo = listenerMap.get( className );
+ ListenerInfo listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo != null )
{
- if ( listenerInfo.remove( filter, listener ) )
- {
- listenerMap.remove( className );
- m_context.removeServiceListener( listenerInfo );
- }
+ listenerMap.remove( serviceFilterString );
Review comment:
You are right, this is not correct. I think it is missing the removal of the ExtendedServiceListener from the ListenerInfo first, and only removing it from the map if the ListenerInfo is empty.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] glimmerveen commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
glimmerveen commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r412465731
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
##########
@@ -2283,8 +2283,8 @@ else if ( m_dependencyMetadata.getScope() == ReferenceScope.prototype_required )
null, getName(), initialActive, refMap, classFilterString, eventFilter,
initialReferenceFilterString );
ServiceTracker<T, RefPair<S, T>, ExtendedServiceEvent> tracker = new ServiceTracker<>(
- bundleContext, m_customizer, initialActive, m_componentManager.getActivator(), eventFilter,
Review comment:
Good catch, will remove the creation of the eventFilter as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] glimmerveen commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
glimmerveen commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r412444967
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java
##########
@@ -98,133 +98,82 @@
private static class ListenerInfo implements ServiceListener
{
- private Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap = new HashMap<>();
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = new ArrayList<>();
@Override
public void serviceChanged(ServiceEvent event)
{
ServiceReference<?> ref = event.getServiceReference();
- ExtendedServiceEvent extEvent = null;
- ExtendedServiceEvent endMatchEvent = null;
- Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap;
+ ExtendedServiceEvent extEvent = new ExtendedServiceEvent(event);
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners;
synchronized ( this )
{
- filterMap = this.filterMap;
+ listeners = this.listeners;
}
- for ( Map.Entry<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> entry : filterMap.entrySet() )
+
+ for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : listeners)
{
- Filter filter = entry.getKey();
- if ( filter == null || filter.match( ref ) )
- {
- if ( extEvent == null )
- {
- extEvent = new ExtendedServiceEvent( event );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( extEvent );
- }
- }
- else if ( event.getType() == ServiceEvent.MODIFIED )
- {
- if ( endMatchEvent == null )
- {
- endMatchEvent = new ExtendedServiceEvent( ServiceEvent.MODIFIED_ENDMATCH, ref );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( endMatchEvent );
- }
- }
+ forwardTo.serviceChanged( extEvent );
}
+
if ( extEvent != null )
{
extEvent.activateManagers();
}
- if ( endMatchEvent != null )
- {
- endMatchEvent.activateManagers();
- }
}
- public synchronized void add(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized void add(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- filterMap = new HashMap<>( filterMap );
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners == null )
- {
- listeners = Collections.<ExtendedServiceListener<ExtendedServiceEvent>> singletonList( listener );
- }
- else
- {
- listeners = new ArrayList<>( listeners );
- listeners.add( listener );
- }
- filterMap.put( filter, listeners );
+ listeners = new ArrayList<>(listeners);
+ listeners.add(listener);
}
- public synchronized boolean remove(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized boolean remove(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners != null )
- {
- filterMap = new HashMap<>( filterMap );
- listeners = new ArrayList<>( listeners );
- listeners.remove( listener );
- if ( listeners.isEmpty() )
- {
- filterMap.remove( filter );
- }
- else
- {
- filterMap.put( filter, listeners );
- }
- }
- return filterMap.isEmpty();
+ listeners = new ArrayList<>(listeners);
+ listeners.remove(listener);
+ return listeners.isEmpty();
}
}
@Override
- public void addServiceListener(String classNameFilter, Filter eventFilter,
+ public void addServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
ListenerInfo listenerInfo;
synchronized ( listenerMap )
{
- logger.log( LogService.LOG_DEBUG, "classNameFilter: " + classNameFilter + " event filter: " + eventFilter, null);
- listenerInfo = listenerMap.get( classNameFilter );
+ logger.log( LogService.LOG_DEBUG, "serviceFilterString: " + serviceFilterString, null);
+ listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo == null )
{
listenerInfo = new ListenerInfo();
- listenerMap.put( classNameFilter, listenerInfo );
+ listenerMap.put( serviceFilterString, listenerInfo );
try
{
- m_context.addServiceListener( listenerInfo, classNameFilter );
+ m_context.addServiceListener( listenerInfo, serviceFilterString );
}
catch ( InvalidSyntaxException e )
{
throw (IllegalArgumentException) new IllegalArgumentException(
"invalid class name filter" ).initCause( e );
}
}
+ listenerInfo.add(listener);
}
- listenerInfo.add( eventFilter, listener );
}
@Override
- public void removeServiceListener(String className, Filter filter,
+ public void removeServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
synchronized ( listenerMap )
{
- ListenerInfo listenerInfo = listenerMap.get( className );
+ ListenerInfo listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo != null )
{
- if ( listenerInfo.remove( filter, listener ) )
- {
- listenerMap.remove( className );
- m_context.removeServiceListener( listenerInfo );
- }
+ listenerMap.remove( serviceFilterString );
Review comment:
Perhaps it is related to current approach of effectively aggregating multiple references into a single ServiceListener. As this PR aims to remove this aggregation, perhaps this is something that can be further simplified?
EDIT: I don't think my assumption here is accurate, it looks more like that all ServiceListeners needed to construct components from a single bundle are aggregated through this class. If multiple components have the same reference, the same ServiceListener is reused?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] tjwatson commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
tjwatson commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r412440777
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java
##########
@@ -98,133 +98,82 @@
private static class ListenerInfo implements ServiceListener
{
- private Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap = new HashMap<>();
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = new ArrayList<>();
@Override
public void serviceChanged(ServiceEvent event)
{
ServiceReference<?> ref = event.getServiceReference();
- ExtendedServiceEvent extEvent = null;
- ExtendedServiceEvent endMatchEvent = null;
- Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap;
+ ExtendedServiceEvent extEvent = new ExtendedServiceEvent(event);
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners;
synchronized ( this )
{
- filterMap = this.filterMap;
+ listeners = this.listeners;
}
- for ( Map.Entry<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> entry : filterMap.entrySet() )
+
+ for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : listeners)
{
- Filter filter = entry.getKey();
- if ( filter == null || filter.match( ref ) )
- {
- if ( extEvent == null )
- {
- extEvent = new ExtendedServiceEvent( event );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( extEvent );
- }
- }
- else if ( event.getType() == ServiceEvent.MODIFIED )
- {
- if ( endMatchEvent == null )
- {
- endMatchEvent = new ExtendedServiceEvent( ServiceEvent.MODIFIED_ENDMATCH, ref );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( endMatchEvent );
- }
- }
+ forwardTo.serviceChanged( extEvent );
}
+
if ( extEvent != null )
{
extEvent.activateManagers();
}
- if ( endMatchEvent != null )
- {
- endMatchEvent.activateManagers();
- }
}
- public synchronized void add(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized void add(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- filterMap = new HashMap<>( filterMap );
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners == null )
- {
- listeners = Collections.<ExtendedServiceListener<ExtendedServiceEvent>> singletonList( listener );
- }
- else
- {
- listeners = new ArrayList<>( listeners );
- listeners.add( listener );
- }
- filterMap.put( filter, listeners );
+ listeners = new ArrayList<>(listeners);
+ listeners.add(listener);
}
- public synchronized boolean remove(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
+ public synchronized boolean remove(ExtendedServiceListener<ExtendedServiceEvent> listener)
{
- List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = filterMap.get( filter );
- if ( listeners != null )
- {
- filterMap = new HashMap<>( filterMap );
- listeners = new ArrayList<>( listeners );
- listeners.remove( listener );
- if ( listeners.isEmpty() )
- {
- filterMap.remove( filter );
- }
- else
- {
- filterMap.put( filter, listeners );
- }
- }
- return filterMap.isEmpty();
+ listeners = new ArrayList<>(listeners);
+ listeners.remove(listener);
+ return listeners.isEmpty();
}
}
@Override
- public void addServiceListener(String classNameFilter, Filter eventFilter,
+ public void addServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
ListenerInfo listenerInfo;
synchronized ( listenerMap )
{
- logger.log( LogService.LOG_DEBUG, "classNameFilter: " + classNameFilter + " event filter: " + eventFilter, null);
- listenerInfo = listenerMap.get( classNameFilter );
+ logger.log( LogService.LOG_DEBUG, "serviceFilterString: " + serviceFilterString, null);
+ listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo == null )
{
listenerInfo = new ListenerInfo();
- listenerMap.put( classNameFilter, listenerInfo );
+ listenerMap.put( serviceFilterString, listenerInfo );
try
{
- m_context.addServiceListener( listenerInfo, classNameFilter );
+ m_context.addServiceListener( listenerInfo, serviceFilterString );
}
catch ( InvalidSyntaxException e )
{
throw (IllegalArgumentException) new IllegalArgumentException(
"invalid class name filter" ).initCause( e );
}
}
+ listenerInfo.add(listener);
}
- listenerInfo.add( eventFilter, listener );
}
@Override
- public void removeServiceListener(String className, Filter filter,
+ public void removeServiceListener(String serviceFilterString,
ExtendedServiceListener<ExtendedServiceEvent> listener)
{
synchronized ( listenerMap )
{
- ListenerInfo listenerInfo = listenerMap.get( className );
+ ListenerInfo listenerInfo = listenerMap.get( serviceFilterString );
if ( listenerInfo != null )
{
- if ( listenerInfo.remove( filter, listener ) )
- {
- listenerMap.remove( className );
- m_context.removeServiceListener( listenerInfo );
- }
+ listenerMap.remove( serviceFilterString );
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] tjwatson commented on a change in pull request #19: [FELIX-6161] Replace local filtering of references in favour of relying on filtered ServiceListener callbacks
Posted by GitBox <gi...@apache.org>.
tjwatson commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r413039089
##########
File path: scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java
##########
@@ -98,132 +98,83 @@
private static class ListenerInfo implements ServiceListener
{
- private Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap = new HashMap<>();
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = new ArrayList<>();
@Override
public void serviceChanged(ServiceEvent event)
{
ServiceReference<?> ref = event.getServiceReference();
- ExtendedServiceEvent extEvent = null;
- ExtendedServiceEvent endMatchEvent = null;
- Map<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap;
+ ExtendedServiceEvent extEvent = new ExtendedServiceEvent(event);
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners;
synchronized ( this )
{
- filterMap = this.filterMap;
+ listeners = this.listeners;
}
- for ( Map.Entry<Filter, List<ExtendedServiceListener<ExtendedServiceEvent>>> entry : filterMap.entrySet() )
+
+ for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : listeners)
{
- Filter filter = entry.getKey();
- if ( filter == null || filter.match( ref ) )
- {
- if ( extEvent == null )
- {
- extEvent = new ExtendedServiceEvent( event );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( extEvent );
- }
- }
- else if ( event.getType() == ServiceEvent.MODIFIED )
- {
- if ( endMatchEvent == null )
- {
- endMatchEvent = new ExtendedServiceEvent( ServiceEvent.MODIFIED_ENDMATCH, ref );
- }
- for ( ExtendedServiceListener<ExtendedServiceEvent> forwardTo : entry.getValue() )
- {
- forwardTo.serviceChanged( endMatchEvent );
- }
- }
+ forwardTo.serviceChanged( extEvent );
}
+
if ( extEvent != null )
{
extEvent.activateManagers();
}
- if ( endMatchEvent != null )
- {
- endMatchEvent.activateManagers();
- }
}
- public synchronized void add(Filter filter, ExtendedServiceListener<ExtendedServiceEvent> listener)
Review comment:
Removal of the Filter param leaves an unused import above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org