Bug 326918

Comment 1 Thomas Watson 2010-10-04 10:20:19 EDT

In this scenario what class name is being used to the call:
 isAssignableTo(Bundle bundle, String className)

Is it foo.Itf1 or bar.Itf2?

I assume Bundle A is importing both foo and bar packages?  A is considered the producer of the service.  If A is importing both foo and bar I do not see how the producerSource (the package source or wire for package foo or bar) would be null and cause us to enter into case 5.

Comment 2 Guillaume Nodet 2010-10-04 11:06:55 EDT

Sorry, i meant bundle C imports the bar package, but not foo.  The real use case is a bit more complicated, but in the end C exports a service under a class which package is not imported, which leads to the producerSource being null.

Comment 3 Guillaume Nodet 2010-10-04 11:09:05 EDT

Sorry, i messed up again my example.
it's A which does not see foo.Itf1, while C imports the service and exports both packages.

Comment 4 Thomas Watson 2010-10-04 11:20:09 EDT

Sorry Guillaume, could you rephrase comment 0 with the correct information.  I think I could give a good guess at what you are doing, but it is still a bit unclear to me.

Comment 5 Guillaume Nodet 2010-10-04 11:35:14 EDT

* Bundle C exports several packages including foo and bar with:
     class foo.MyClass implements bar.MyInterface
  this bundle also registers a service listener of bar.MyInterface services
* Bundle B is a blueprint extender which has a ServiceFactory implementation which is used
    to expose a service of class foo.MyClass from bundle A under both foo.MyClass and bar.MyInterface 
    in the OSGi registry
* Bundle A imports bar package (so that A can see bar.MyInterface) but not foo (so that A can't see foo.MyClass)

The real world example is a bit more complicated than that, as you may wonder how B can export a service using foo.MyClass which isn't can't be loaded using A's bundleContext.   In that case, it's done through a custom blueprint namespace handler registered by C (so visible to B) and referenced by A through a custom namespace extension.  

Also, I think in that case, the bar.MyInterface is not important, as the it really comes down to A exporting a service under a class name in a package it can not see.  Though the whole class space is consistent in our case, because the blueprint extender verifies the consistency (either the class is the same or it can't be seen at all).

I can point you to the real code which comes from Karaf if you want, but you'd have to compile it, or i can give you a prebuilt version if you want to debug through the problem.

Comment 6 Thomas Watson 2010-10-04 11:46:46 EDT

Have you seen similar behavior on Felix.  Richard Hall and I have discussed this type of scenario several times.  I am pretty sure Felix and Equinox behave the same in this case.

I assume in your case B does not import the foo package either?

Comment 7 Guillaume Nodet 2010-10-04 12:04:54 EDT

Yeah, Felix works in that case, but it's mostly because there's a factory involved i think.  Actually, as there's a factory involved, Felix does not perform checks on the classes itself, which makes sense because the factory does not have to come from the same bundle as the service itself.
So i agree this could potentially lead to inconsistency, but no one can really know in that case.

Comment 8 Thomas Watson 2010-10-04 12:15:50 EDT

I just talked with Richard on this issue.  It looks like Felix added another check to the logic:

  if (factoryClass is not from the registrant bundle) {
    return true;
  }

So in this case the factory object class comes from B (not A) and A's bundle context registered the service.  I will have to think on this a bit more.  It a much more lenient behavior than the specification mandates.  In the end I would like Felix and Equinox to have the same behavior in this regard.

Comment 9 Thomas Watson 2010-10-06 10:38:52 EDT

Created attachment 180337 [details]
patch

Here is a patch that does an extra check for ServiceFactory service classes.  If the registrant bundle does not have a package wire for the service package and the service class is a type of ServiceFactory then we check to see if the service class is defined by a bundle class loader; if that bundle is not equal to the registrant bundle then we allow the assignment.  Kinda wacky.

Comment 10 Richard S. Hall 2010-10-06 11:50:38 EDT

The basic idea was to assume that if a service factory was in use and it came from a different bundle than the actual service provider (i.e., the context used to register the service), then assume that the extender pattern is at play and allow it because the extender must know what it is doing.

Admittedly, this won't always be correct, but we are trying to be nice. If we run across a scenario where it fails, then we can try to adjust accordingly at that time.

Comment 11 Thomas Watson 2010-10-06 13:13:31 EDT

Guillaume and Richard, do you think an OSGi bug should be opened to loosen this check in the specification?  It may be a tough sell since the specification will have to be loosened in a way that can allow invalid services to be returned.

Clearly the OSGi CT does not test this case otherwise we would start failing, but it is always possible a testcase could get added for this.

Comment 12 Guillaume Nodet 2010-10-06 13:31:22 EDT

Well, I think this use case is valid, though one could argue the bundle needs to import the packages containing the services exported by the extender, but I don't think it would be a good idea.   I definitely support opening a bug and try to get that through if possible.

Comment 13 Richard S. Hall 2010-10-06 14:09:59 EDT

It is not clear if it should be spec'ed. It is a heuristic and a fallible one at that. To spec this, you'd like need to spec all of the other checks we do too, since this is only one of them and it comes after some others.

Comment 14 Thomas Watson 2010-10-06 15:44:53 EDT

(In reply to comment #13)
> It is not clear if it should be spec'ed. It is a heuristic and a fallible one
> at that. To spec this, you'd like need to spec all of the other checks we do
> too, since this is only one of them and it comes after some others.

I agree, this is not something I would enjoy spec'ing.  For one thing it would make the description of ServiceReference.isAssignableTo incomprehensible to almost everyone, except perhaps Richard and myself ;-)

Guillaume, it may at least be worth your while to bring this up with CPEG to discuss your usecase and see if the expert group has another approach to solve your particular problem.

I will plan to release this patch (or something similar) for M3 after I have done some more testing.  Guillaume it would also be good if you could try the patch out in your actual scenario to see if it addresses your usecase.

Comment 15 Thomas Watson 2010-10-11 09:53:27 EDT

Patch released to HEAD.

Comment 16 Scott Lewis 2011-02-23 19:00:41 EST

As per the recent discussion on osgi-dev, we/ECF are using the capability introduced by this patch/bug for Remote Service Admin proxy creation (i.e. using ServiceFactory).  We've got an addition for ECF RSA that already uses it, and it works fine on 3.7m4.

But we/ECF would like to also support running on Equinox < 3.7m3...at least in a 'degraded' form...as I assume lots of consumers will be using Equinox 3.6.X for some time.  To do this, we need to have some way to detect the versions of Equinox that do *not* have this patch, and use a different BundleContext to register our proxy ServiceFactory (long story here, see https://mail.osgi.org/pipermail/osgi-dev/2011-February/003021.html

So...how best to detect that Equinox is a version that does *not* have this fix?  Should we consult some version of some core bundle, or a system prop, or some such?  Thanks for any info.

Comment 17 Thomas Watson 2011-02-24 09:03:36 EST

(In reply to comment #16)
> As per the recent discussion on osgi-dev, we/ECF are using the capability
> introduced by this patch/bug for Remote Service Admin proxy creation (i.e.
> using ServiceFactory).  We've got an addition for ECF RSA that already uses it,
> and it works fine on 3.7m4.
> 
> But we/ECF would like to also support running on Equinox < 3.7m3...at least in
> a 'degraded' form...as I assume lots of consumers will be using Equinox 3.6.X
> for some time.  To do this, we need to have some way to detect the versions of
> Equinox that do *not* have this patch, and use a different BundleContext to
> register our proxy ServiceFactory (long story here, see
> https://mail.osgi.org/pipermail/osgi-dev/2011-February/003021.html
> 

I am familiar with that thread ;-)

> So...how best to detect that Equinox is a version that does *not* have this
> fix?  Should we consult some version of some core bundle, or a system prop, or
> some such?  Thanks for any info.

code like this should work:

BundleContext context = ...;
Bundle systemBundle = context.getBundle(0);
String systemBSN = systemBundle.getSymbolicName();
if ("org.eclipse.osgi".equals(systemBSN) {
  // Version fixedVersion = new Version("3.7.0");
  // running on equinox; check the version
  // note that getVersion() is new to equinox 3.5 (OSGi R4.2 spec)
  Version systemVersion = systemBundle.getVersion();
  if (systemVersion.compareTo(fixedVersion) < 0) {
     // running on equinox < 3.7.0
  }
}

Caution, uncompiled and untested code, use at your own risk ;-)

Comment 18 Scott Lewis 2011-02-24 10:32:59 EST

(In reply to comment #17)
<stuff deleted>
> > some such?  Thanks for any info.
> 
> code like this should work:
> 
> BundleContext context = ...;
> Bundle systemBundle = context.getBundle(0);
> String systemBSN = systemBundle.getSymbolicName();
> if ("org.eclipse.osgi".equals(systemBSN) {
>   // Version fixedVersion = new Version("3.7.0");
>   // running on equinox; check the version
>   // note that getVersion() is new to equinox 3.5 (OSGi R4.2 spec)
>   Version systemVersion = systemBundle.getVersion();
>   if (systemVersion.compareTo(fixedVersion) < 0) {
>      // running on equinox < 3.7.0
>   }
> }
> 
> Caution, uncompiled and untested code, use at your own risk ;-)

Thanks...I always do.  :)