Uploaded image for project: 'Sling'
  1. Sling
  2. SLING-5664

Models: OSGiServiceInjector does not consider service.ranking

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: Sling Models Impl 1.2.6
    • Fix Version/s: Sling Models Impl 1.2.8
    • Component/s: Extensions
    • Labels:
      None

      Description

      The injector is using BundleContext.getServiceReferences(Class, String) or SlingScriptHelper.getServices(Class, String) (https://github.com/apache/sling/blob/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/OSGiServiceInjector.java#L97). From the returned array it just takes the first item.
      Unfortunately the order in that array is not-deterministic, especially it is not necessarily that way that items with a higher service.ranking are returned first.

      1. SLING-5664-v01.patch
        23 kB
        Konrad Windszus

        Issue Links

          Activity

          Hide
          kwin Konrad Windszus added a comment -

          The implementation in `OsgiServiceReferenceUtils.getServiceReference(...) seems to solve exactly that issue (https://src.springframework.org/svn/spring-osgi/trunk/core/src/main/java/org/springframework/osgi/util/OsgiServiceReferenceUtils.java).

          Show
          kwin Konrad Windszus added a comment - The implementation in `OsgiServiceReferenceUtils.getServiceReference(...) seems to solve exactly that issue ( https://src.springframework.org/svn/spring-osgi/trunk/core/src/main/java/org/springframework/osgi/util/OsgiServiceReferenceUtils.java ).
          Hide
          kwin Konrad Windszus added a comment - - edited

          Attached is a patch which makes sure that

          1. for array injections the service with the highest ranking is returned first (and afterwards the array is sorted descending by service order)
          2. and for single value injections only the service with the highest ranking is injected

          Also I added an IT to verify this behaviour.

          Due to SLING-5665 I had to remove usage of SlingScriptHelper in the injector completely as this does currently not give any opportunity to figure out the service ranking.
          Justin Edelson and Stefan Seifert can you both have a look?

          Show
          kwin Konrad Windszus added a comment - - edited Attached is a patch which makes sure that for array injections the service with the highest ranking is returned first (and afterwards the array is sorted descending by service order) and for single value injections only the service with the highest ranking is injected Also I added an IT to verify this behaviour. Due to SLING-5665 I had to remove usage of SlingScriptHelper in the injector completely as this does currently not give any opportunity to figure out the service ranking. Justin Edelson and Stefan Seifert can you both have a look?
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          I had to remove usage of SlingScriptHelper in the injector completely...

          I suppose there is a reason for OSGiServiceInjector to use either SlingScriptHelper or BundleContext to get services, it would be good to find out what that reason is before removing the use of SlingScriptHelper.

          The alternative is to require SLING-5665 which would (IIUC) allow to keep using SlingScriptHelper.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - I had to remove usage of SlingScriptHelper in the injector completely... I suppose there is a reason for OSGiServiceInjector to use either SlingScriptHelper or BundleContext to get services, it would be good to find out what that reason is before removing the use of SlingScriptHelper . The alternative is to require SLING-5665 which would (IIUC) allow to keep using SlingScriptHelper .
          Hide
          kwin Konrad Windszus added a comment -

          The reason for `SlingScriptHelper` to be used in the past (whenever possible, i.e. when the adaptable is a request and the SlingScriptHelper is available at all) is that it will release the reference towards the OSGi service sooner (as soon as the request is closed). When using the `BundleContext` there is a mechanism regarding phantom references (therefore those services will be released later, depending on the garbage collection).

          So basically we have two options:

          1. rely on the fix from SLING-5665 (which makes this fix incomplete/not working, unless you have the most recent version of Sling Scripting Core, 2.0.38)
          2. remove usage of `SlingScriptHelper` as done in the attached script and live with the fact, that service references live a bit longer

          I would be in favor of 2.).

          Show
          kwin Konrad Windszus added a comment - The reason for `SlingScriptHelper` to be used in the past (whenever possible, i.e. when the adaptable is a request and the SlingScriptHelper is available at all) is that it will release the reference towards the OSGi service sooner (as soon as the request is closed). When using the `BundleContext` there is a mechanism regarding phantom references (therefore those services will be released later, depending on the garbage collection). So basically we have two options: rely on the fix from SLING-5665 (which makes this fix incomplete/not working, unless you have the most recent version of Sling Scripting Core, 2.0.38) remove usage of `SlingScriptHelper` as done in the attached script and live with the fact, that service references live a bit longer I would be in favor of 2.).
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          Thanks for clarifying, I agree with not using SlingScriptrHelper anymore, unless someone else objects.

          Those services will be released later, depending on the garbage collection

          Ok, if needed we can later add a mechanism to register service references for cleanup when the request is closed. That would be cleaner than the trick that you describe, which was not obvious to me by looking at the code.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - Thanks for clarifying, I agree with not using SlingScriptrHelper anymore, unless someone else objects. Those services will be released later, depending on the garbage collection Ok, if needed we can later add a mechanism to register service references for cleanup when the request is closed. That would be cleaner than the trick that you describe, which was not obvious to me by looking at the code.
          Hide
          kwin Konrad Windszus added a comment -

          Good idea, I created SLING-5668 for that.

          Show
          kwin Konrad Windszus added a comment - Good idea, I created SLING-5668 for that.
          Hide
          kwin Konrad Windszus added a comment -

          Fixed in r1739788.

          Show
          kwin Konrad Windszus added a comment - Fixed in r1739788 .
          Hide
          kwin Konrad Windszus added a comment -

          And a followup commit in r1739844

          Show
          kwin Konrad Windszus added a comment - And a followup commit in r1739844

            People

            • Assignee:
              kwin Konrad Windszus
              Reporter:
              kwin Konrad Windszus
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development