Comment 1 Scott Lewis 2010-05-04 08:52:43 EDT

(In reply to comment #0)
org.eclipse.ecf.osgi.services.distribution.AbstractHostContainerFinder.createMatchingContainer(ContainerTypeDescription,
> ServiceReference, String[], String[], String[]) tries to create a
> org.eclipse.ecf.remoteservice.RemoteServiceContainer with a discovery container
> (e.g. JmDNS) which causes the isNotNull() assertion in RemoteServiceContainer
> to fail. Filtering containerTypeDescriptions on
> "IRemoteServiceContainerAdapter" prevents this.


It would be good to understand why the Discovery Container is being considered/included here (since it's clearly not the right type).

Guarding the call to createContainer in createMatchingContainer there is this logic:

if (matchHostSupportedConfigTypes(requiredConfigs,
		containerTypeDescription)
		&& matchHostSupportedIntents(requiredIntents,
				containerTypeDescription)) {
	try {
		IContainer container = createContainer(serviceReference,
				containerTypeDescription);
		return new RemoteServiceContainer(container);
	} catch (Exception e) {
...


In matchHostSupportedConfigTypes there is getSupportedConfigTypes, and getSupportedConfigTypes ends up calling containerTypeDescription.getSupportedConfigs() on all container type descriptions in order to decide whether to use/create a container of the given type.  So for the Discovery Container getSupportedConfigs container should return null.

Comment 2 Markus Kuppe 2010-05-04 10:35:42 EDT

If both requiredConfigs and requiredIntents are null in org.eclipse.ecf.osgi.services.distribution.AbstractHostContainerFinder.createMatchingContainer(ContainerTypeDescription, ServiceReference, String[], String[], String[]) the filter methods org.eclipse.ecf.osgi.services.distribution.AbstractHostContainerFinder.matchHostSupportedConfigTypes(String[], ContainerTypeDescription) and org.eclipse.ecf.osgi.services.distribution.AbstractHostContainerFinder.matchHostSupportedIntents(String[], ContainerTypeDescription) return true.

Comment 3 Scott Lewis 2010-05-04 10:38:44 EDT

(In reply to comment #2)
> If both requiredConfigs and requiredIntents are null in
> org.eclipse.ecf.osgi.services.distribution.AbstractHostContainerFinder.createMatchingContainer(ContainerTypeDescription,
> ServiceReference, String[], String[], String[]) the filter methods
> org.eclipse.ecf.osgi.services.distribution.AbstractHostContainerFinder.matchHostSupportedConfigTypes(String[],
> ContainerTypeDescription) and
> org.eclipse.ecf.osgi.services.distribution.AbstractHostContainerFinder.matchHostSupportedIntents(String[],
> ContainerTypeDescription) return true.


requiredConfigs typically should not be null

Comment 4 Markus Kuppe 2010-05-04 10:45:12 EDT

It's null because the servicereference in org.eclipse.ecf.internal.osgi.services.distribution.EventHookImpl.handleRegisteredServiceEvent(ServiceReference, Collection) does not set org.eclipse.ecf.osgi.services.distribution.IDistributionConstants.SERVICE_EXPORTED_CONFIGS. 

If SERVICE_EXPORTED_CONFIGS is mandatory for a remote service to be set, we might wanna fail more gracefully in handleRegisteredServiceEvent.

Comment 5 Markus Kuppe 2010-05-04 10:46:47 EDT

Btw. from the code it appears as if SERVICE_EXPORTED_CONFIGS is not strictly required:

// Get optional service property for exported configs
String[] exportedConfigs = getExportedConfigs(serviceReference);

Comment 6 Scott Lewis 2010-05-04 12:30:37 EDT

Assigning to myself.  I'm looking at the spec, and it's not at all clear to me so far whether service.exported.configs is required.  Practically speaking, though, it seems to be required (because it's the only way a distribution provider...like ECF...can determine whether a given remote registration should be handled by *that* provider).  But I'm looking through spec now (irrespective of my comment in the code).

Even if it is required, though, the matching should indeed fail gracefully (rather than creating a container and failing to cast it) if the service.exported.configs is null, because for a given provider (ECF) it has to have some indication of whether that provider should be used...and null doesn't help any provider decide that.

So I'll fix this over the next few minutes.

Comment 7 Markus Kuppe 2010-05-04 12:52:41 EDT

(In reply to comment #6)
> Assigning to myself.  I'm looking at the spec, and it's not at all clear to me
> so far whether service.exported.configs is required.  Practically speaking,
> though, it seems to be required (because it's the only way a distribution
> provider...like ECF...can determine whether a given remote registration should
> be handled by *that* provider).  But I'm looking through spec now (irrespective
> of my comment in the code).

Why not interpret it as a wildcard with meaning that all distribution provider deployed may handle this service? Unless there is an explicit wildcard defined in as part of the spec.

Comment 8 Scott Lewis 2010-05-04 13:02:22 EDT

(In reply to comment #7)
> (In reply to comment #6)
> > Assigning to myself.  I'm looking at the spec, and it's not at all clear to me
> > so far whether service.exported.configs is required.  Practically speaking,
> > though, it seems to be required (because it's the only way a distribution
> > provider...like ECF...can determine whether a given remote registration should
> > be handled by *that* provider).  But I'm looking through spec now (irrespective
> > of my comment in the code).
> 
> Why not interpret it as a wildcard with meaning that all distribution provider
> deployed may handle this service? Unless there is an explicit wildcard defined
> in as part of the spec.

I would only do this if this semantics is defined in the spec...and so far I can't find it (but am continuing to look in r4.cmpn.pdf chap 13).  Giving the spec the semantics of having all distribution providers handle such a registration is more than a little risky though...since it could result in a number of providers implicitly distributing any given service (and making it available remotely is potentially a big security risk, of course).  But I'm looking at the spec now.

Comment 9 Scott Lewis 2010-05-04 15:05:06 EDT

Fix released to HEAD.  The fix involved implementing a defaultConfigType(s) that are used if the required config types is null (not specified), or no match to specified config types is given (and <<nodefaults>> is *not* present...as per chap 13 in spec).