karlpauls commented on a change in pull request #13:
URL:
https://github.com/apache/sling-org-apache-sling-scripting-core/pull/13#discussion_r802067459
##########
File path:
src/main/java/org/apache/sling/scripting/core/impl/bundled/AbstractBundledRenderUnit.java
##########
@@ -101,83 +95,37 @@ public BundleContext getBundleContext() {
return scriptExtension;
}
+ @Override
+ public @NotNull ServiceCache getServiceCache() {
+ return serviceCache;
+ }
+
@Override
@Nullable
@SuppressWarnings("unchecked")
public <T> T getService(@NotNull String className) {
- LOG.debug("Attempting to load class {} as an OSGi service.",
className);
- T result = (this.services == null ? null : (T)
this.services.get(className));
- if (result == null) {
- final ServiceReference<?> ref =
this.bundleContext.getServiceReference(className);
- if (ref != null) {
- result = (T) this.bundleContext.getService(ref);
- if (result != null) {
- if (this.services == null) {
- this.services = new HashMap<>();
- }
- if (this.references == null) {
- this.references = new ArrayList<>();
- }
- this.references.add(ref);
- this.services.put(className, result);
- return result;
- }
- }
+ try {
+ ClassLoader bundleClassloader =
getBundle().adapt(BundleWiring.class).getClassLoader();
+ return (T)
serviceCache.getService(bundleClassloader.loadClass(className));
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to retrieve a service of type " + className + "
for bundled script " + path, e);
}
- return result;
+ return null;
}
@Override
@Nullable
@SuppressWarnings("unchecked")
public <T> T[] getServices(@NotNull String className, @Nullable String
filter) {
- T[] result = null;
try {
- final ServiceReference<?>[] refs =
this.bundleContext.getServiceReferences(className, filter);
-
- if (refs != null) {
- // sort by service ranking (lowest first) (see
ServiceReference#compareTo(Object))
- List<ServiceReference<?>> localReferences =
Arrays.asList(refs);
- Collections.sort(localReferences);
- // get the highest ranking first
- Collections.reverse(localReferences);
-
- final List<T> objects = new ArrayList<>();
- for (ServiceReference<?> reference : localReferences) {
- final T service = (T)
this.bundleContext.getService(reference);
- if (service != null) {
- if (this.references == null) {
- this.references = new ArrayList<>();
- }
- this.references.add(reference);
- objects.add(service);
- }
- }
- if (!objects.isEmpty()) {
- T[] srv = (T[])
Array.newInstance(bundle.loadClass(className), objects.size());
- result = objects.toArray(srv);
- }
- }
- } catch (Exception e) {
- LOG.error(String.format("Unable to retrieve the services of type
%s.", className), e);
+ ClassLoader bundleClassloader =
getBundle().adapt(BundleWiring.class).getClassLoader();
+ return (T[])
serviceCache.getServices(bundleClassloader.loadClass(className), filter);
+ } catch (ClassNotFoundException e) {
Review comment:
Same question here I guess: are we sure we don't have cases where
somebody gets the service to use it via reflection?
##########
File path: src/main/java/org/apache/sling/scripting/core/impl/ServiceCache.java
##########
@@ -63,70 +70,151 @@ public void dispose() {
* @return The service or <code>null</code>
*/
@SuppressWarnings("unchecked")
+ @Nullable
public <ServiceType> ServiceType getService(Class<ServiceType> type) {
- final String key = type.getName();
- Reference reference = this.cache.get(key);
- if (reference == null) {
-
- // get the service
- ServiceReference ref = this.bundleContext.getServiceReference(key);
- if (ref != null) {
- final Object service = this.bundleContext.getService(ref);
- if (service != null) {
- reference = new Reference();
- reference.service = service;
- reference.reference = ref;
- } else {
- ref = null;
- }
- }
-
- // assume missing service
- if (reference == null) {
- reference = NULL_REFERENCE;
+ SortedSet<Reference> references = getCachedReferences(type);
+ for (Reference reference : references) {
+ ServiceType service = (ServiceType) reference.getService();
+ if (service != null) {
+ return service;
}
+ }
+ return null;
+ }
- // check to see whether another thread has not done the same thing
- synchronized (this) {
- Reference existing = this.cache.get(key);
- if (existing == null) {
- this.cache.put(key, reference);
- ref = null;
- } else {
- reference = existing;
+ @SuppressWarnings("unchecked")
+ @Nullable
+ public <ServiceType> ServiceType[] getServices(Class<ServiceType> type,
String filter) {
+ List<ServiceType> result = new ArrayList<>();
+ try {
+ SortedSet<Reference> cachedReferences = getCachedReferences(type);
+ final Collection<ServiceReference<ServiceType>> filteredReferences
= this.bundleContext.getServiceReferences(type, filter);
+ if (!filteredReferences.isEmpty()) {
+ List<ServiceReference<ServiceType>> localFilteredReferences =
new ArrayList<>(filteredReferences);
+ Collections.sort(localFilteredReferences);
+ // get the highest ranking first
+ Collections.reverse(localFilteredReferences);
+ for (ServiceReference<ServiceType> serviceReference :
localFilteredReferences) {
+ Reference lookup = new Reference(serviceReference);
+ if (cachedReferences.contains(lookup)) {
+ for (Reference reference : cachedReferences) {
+ if
(serviceReference.equals(reference.getServiceReference())) {
+ ServiceType service = (ServiceType)
reference.getService();
+ if (service != null) {
+ result.add(service);
+ }
+ break;
+ }
+ }
+ } else {
+ // concurrent change; restart
+ return getServices(type, filter);
+ }
}
}
-
- // unget the service if another thread was faster
- if (ref != null) {
- this.bundleContext.ungetService(ref);
- }
+ } catch (InvalidSyntaxException e) {
+ LOGGER.error(String.format("Unable to retrieve the services of
type %s.", type.getName()), e);
}
-
- // return whatever we got (which may be null)
- return (ServiceType) reference.service;
+ if (!result.isEmpty()) {
+ ServiceType[] srv = (ServiceType[]) Array.newInstance(type,
result.size());
+ return result.toArray(srv);
+ }
+ return null;
}
/**
* @see
org.osgi.framework.ServiceListener#serviceChanged(org.osgi.framework.ServiceEvent)
*/
public void serviceChanged(ServiceEvent event) {
- final String[] objectClasses =
(String[])event.getServiceReference().getProperty(Constants.OBJECTCLASS);
- if ( objectClasses != null) {
- for(final String key : objectClasses) {
- Reference ref = null;
- synchronized ( this ) {
- ref = this.cache.remove(key);
+ ServiceReference<?> serviceReference = event.getServiceReference();
+ final String[] objectClasses = (String[])
serviceReference.getProperty(Constants.OBJECTCLASS);
+ if (objectClasses != null) {
+ for (final String key : objectClasses) {
+ SortedSet<Reference> references;
+ synchronized (this) {
+ references = this.cache.remove(key);
+ if (references != null) {
+ for (Reference reference : references) {
+
bundleContext.ungetService(reference.getServiceReference());
+ }
+ }
}
- if ( ref != null && ref != NULL_REFERENCE ) {
- this.bundleContext.ungetService(ref.reference);
+ }
+ }
+ }
+
+ private <ServiceType> SortedSet<Reference>
getCachedReferences(Class<ServiceType> type) {
+ String key = type.getName();
+ SortedSet<Reference> references = cache.get(key);
+ if (references == null) {
+ references = Collections.synchronizedSortedSet(new TreeSet<>((o1,
o2) -> -1 * o1.compareTo(o2)));
+ try {
+ Collection<ServiceReference<ServiceType>> serviceReferences =
this.bundleContext.getServiceReferences(type, null);
+ if (!serviceReferences.isEmpty()) {
+ List<ServiceReference<ServiceType>> localReferences = new
ArrayList<>(serviceReferences);
+ Collections.sort(localReferences);
+ Collections.reverse(localReferences);
+ for (ServiceReference<ServiceType> ref : localReferences) {
+ references.add(new Reference(ref));
+ }
+ synchronized (this) {
+ SortedSet<Reference> existing = this.cache.get(key);
+ if (existing != null) {
+ existing.addAll(
+ references.stream().filter(reference ->
!existing.contains(reference)).collect(Collectors.toList()));
Review comment:
I'm not sure the extra filter is needed - are we not already adding to a
set?
##########
File path: src/main/java/org/apache/sling/scripting/core/impl/ServiceCache.java
##########
@@ -63,70 +70,151 @@ public void dispose() {
* @return The service or <code>null</code>
*/
@SuppressWarnings("unchecked")
+ @Nullable
public <ServiceType> ServiceType getService(Class<ServiceType> type) {
- final String key = type.getName();
- Reference reference = this.cache.get(key);
- if (reference == null) {
-
- // get the service
- ServiceReference ref = this.bundleContext.getServiceReference(key);
- if (ref != null) {
- final Object service = this.bundleContext.getService(ref);
- if (service != null) {
- reference = new Reference();
- reference.service = service;
- reference.reference = ref;
- } else {
- ref = null;
- }
- }
-
- // assume missing service
- if (reference == null) {
- reference = NULL_REFERENCE;
+ SortedSet<Reference> references = getCachedReferences(type);
+ for (Reference reference : references) {
+ ServiceType service = (ServiceType) reference.getService();
+ if (service != null) {
+ return service;
}
+ }
+ return null;
+ }
- // check to see whether another thread has not done the same thing
- synchronized (this) {
- Reference existing = this.cache.get(key);
- if (existing == null) {
- this.cache.put(key, reference);
- ref = null;
- } else {
- reference = existing;
+ @SuppressWarnings("unchecked")
+ @Nullable
+ public <ServiceType> ServiceType[] getServices(Class<ServiceType> type,
String filter) {
+ List<ServiceType> result = new ArrayList<>();
+ try {
+ SortedSet<Reference> cachedReferences = getCachedReferences(type);
+ final Collection<ServiceReference<ServiceType>> filteredReferences
= this.bundleContext.getServiceReferences(type, filter);
+ if (!filteredReferences.isEmpty()) {
+ List<ServiceReference<ServiceType>> localFilteredReferences =
new ArrayList<>(filteredReferences);
Review comment:
I guess the extra list isn't really needed, right? We could just use
Arrays.sort with reverse order for the sorting and the for loop already works
with arrays (not that it matters much).
##########
File path:
src/main/java/org/apache/sling/scripting/core/impl/bundled/AbstractBundledRenderUnit.java
##########
@@ -101,83 +95,37 @@ public BundleContext getBundleContext() {
return scriptExtension;
}
+ @Override
+ public @NotNull ServiceCache getServiceCache() {
+ return serviceCache;
+ }
+
@Override
@Nullable
@SuppressWarnings("unchecked")
public <T> T getService(@NotNull String className) {
- LOG.debug("Attempting to load class {} as an OSGi service.",
className);
- T result = (this.services == null ? null : (T)
this.services.get(className));
- if (result == null) {
- final ServiceReference<?> ref =
this.bundleContext.getServiceReference(className);
- if (ref != null) {
- result = (T) this.bundleContext.getService(ref);
- if (result != null) {
- if (this.services == null) {
- this.services = new HashMap<>();
- }
- if (this.references == null) {
- this.references = new ArrayList<>();
- }
- this.references.add(ref);
- this.services.put(className, result);
- return result;
- }
- }
+ try {
+ ClassLoader bundleClassloader =
getBundle().adapt(BundleWiring.class).getClassLoader();
+ return (T)
serviceCache.getService(bundleClassloader.loadClass(className));
+ } catch (ClassNotFoundException e) {
Review comment:
I guess this is quite a change so - previously, we would have allowed to
lookup up services with interfaces that are not wired to the bundle. Now, that
will fail - are we sure that nobody is using this to lookup services that are
then used only via reflection (i.e., don't need the interfaces to be available)?
##########
File path: src/main/java/org/apache/sling/scripting/core/impl/ServiceCache.java
##########
@@ -63,70 +70,151 @@ public void dispose() {
* @return The service or <code>null</code>
*/
@SuppressWarnings("unchecked")
+ @Nullable
public <ServiceType> ServiceType getService(Class<ServiceType> type) {
- final String key = type.getName();
- Reference reference = this.cache.get(key);
- if (reference == null) {
-
- // get the service
- ServiceReference ref = this.bundleContext.getServiceReference(key);
- if (ref != null) {
- final Object service = this.bundleContext.getService(ref);
- if (service != null) {
- reference = new Reference();
- reference.service = service;
- reference.reference = ref;
- } else {
- ref = null;
- }
- }
-
- // assume missing service
- if (reference == null) {
- reference = NULL_REFERENCE;
+ SortedSet<Reference> references = getCachedReferences(type);
+ for (Reference reference : references) {
+ ServiceType service = (ServiceType) reference.getService();
+ if (service != null) {
+ return service;
}
+ }
+ return null;
+ }
- // check to see whether another thread has not done the same thing
- synchronized (this) {
- Reference existing = this.cache.get(key);
- if (existing == null) {
- this.cache.put(key, reference);
- ref = null;
- } else {
- reference = existing;
+ @SuppressWarnings("unchecked")
+ @Nullable
+ public <ServiceType> ServiceType[] getServices(Class<ServiceType> type,
String filter) {
+ List<ServiceType> result = new ArrayList<>();
+ try {
+ SortedSet<Reference> cachedReferences = getCachedReferences(type);
+ final Collection<ServiceReference<ServiceType>> filteredReferences
= this.bundleContext.getServiceReferences(type, filter);
+ if (!filteredReferences.isEmpty()) {
+ List<ServiceReference<ServiceType>> localFilteredReferences =
new ArrayList<>(filteredReferences);
+ Collections.sort(localFilteredReferences);
+ // get the highest ranking first
+ Collections.reverse(localFilteredReferences);
+ for (ServiceReference<ServiceType> serviceReference :
localFilteredReferences) {
+ Reference lookup = new Reference(serviceReference);
+ if (cachedReferences.contains(lookup)) {
+ for (Reference reference : cachedReferences) {
+ if
(serviceReference.equals(reference.getServiceReference())) {
+ ServiceType service = (ServiceType)
reference.getService();
+ if (service != null) {
+ result.add(service);
+ }
+ break;
+ }
+ }
+ } else {
+ // concurrent change; restart
+ return getServices(type, filter);
+ }
}
}
-
- // unget the service if another thread was faster
- if (ref != null) {
- this.bundleContext.ungetService(ref);
- }
+ } catch (InvalidSyntaxException e) {
+ LOGGER.error(String.format("Unable to retrieve the services of
type %s.", type.getName()), e);
}
-
- // return whatever we got (which may be null)
- return (ServiceType) reference.service;
+ if (!result.isEmpty()) {
+ ServiceType[] srv = (ServiceType[]) Array.newInstance(type,
result.size());
+ return result.toArray(srv);
+ }
+ return null;
}
/**
* @see
org.osgi.framework.ServiceListener#serviceChanged(org.osgi.framework.ServiceEvent)
*/
public void serviceChanged(ServiceEvent event) {
- final String[] objectClasses =
(String[])event.getServiceReference().getProperty(Constants.OBJECTCLASS);
- if ( objectClasses != null) {
- for(final String key : objectClasses) {
- Reference ref = null;
- synchronized ( this ) {
- ref = this.cache.remove(key);
+ ServiceReference<?> serviceReference = event.getServiceReference();
+ final String[] objectClasses = (String[])
serviceReference.getProperty(Constants.OBJECTCLASS);
+ if (objectClasses != null) {
+ for (final String key : objectClasses) {
+ SortedSet<Reference> references;
+ synchronized (this) {
+ references = this.cache.remove(key);
+ if (references != null) {
+ for (Reference reference : references) {
+
bundleContext.ungetService(reference.getServiceReference());
+ }
+ }
}
- if ( ref != null && ref != NULL_REFERENCE ) {
- this.bundleContext.ungetService(ref.reference);
+ }
+ }
+ }
+
+ private <ServiceType> SortedSet<Reference>
getCachedReferences(Class<ServiceType> type) {
+ String key = type.getName();
+ SortedSet<Reference> references = cache.get(key);
+ if (references == null) {
+ references = Collections.synchronizedSortedSet(new TreeSet<>((o1,
o2) -> -1 * o1.compareTo(o2)));
Review comment:
Doubt it matters much but should we maybe use a ConcurrentSkipListSet?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
