Add convenient way to get rarely fetched and locally used OSGi services

[HannesWell]

To obtain an instance of a OSGi service often the very basic methods BundleContext.getServiceReference() and BundleContext.(un-)getService() are called directly.
For example the PDE or P2 code base often contain something like the following code snippet:

BundleContext ctx = FrameworkUtil.getBundle(Demo.class).getBundleContext();
ServiceReference<MyService> ref = ctx.getServiceReference(MyService.class);
MyService myService = ctx.getService(ref);
try {
	myService.doSomething();
} finally {
	ctx.ungetService(ref);
}

This is cumbersome and error prone (null-checks are often missing like in the given example). In some cases the service instance is even used outside the try-block. Using the service instance after releasing the reference leads to an incorrect use-count of the service reference, which can lead to follow up errors and maybe even classloader leaks.

Therefore I want to propose a convenient and save way to obtain an instance of a given service that are rarely fetched or where performance does not matter and therefore a ServiceTracker is not necessary or wanted.

The idea is to add new methods to ServiceCaller that returns a auto-closeable supplier of the service instance that can be used in a try-with-resources block. Within the block the get() method returns the service instance and when the block is left the service reference is unget:

try (var service = ServiceCaller.useService(MyService.class)) {
	service.get().doSomething();
}

Of course this does fully prevent one from using the service instance outside the try-block. The intend is to provide methods that make it so simple to obtain a service that such error prone 'convenience hacks' are not necessary.

The new methods are lacking of proper documentation and tests and the names can probably/maybe improved. Nevertheless I wanted to gather general feedback about this proposal. Please let me know if you have improvements.

[github-actions]

Test Results

     24 files  ±0       24 suites  ±0   11m 52s ⏱️ +4s
2 150 tests ±0  2 106 ✔️ +1  44 💤 ±0  0 ❌  - 1 
2 194 runs  ±0  2 150 ✔️ +1  44 💤 ±0  0 ❌  - 1 

Results for commit 9af7f34. ± Comparison against base commit 856f09e.

♻️ This comment has been updated with latest results.

[laeubi]

I would suggest to use an optional here, thats much more convenient and flexible and one can use the "orThrows" methods to give meaningful exception/error messages.

[HannesWell]

I thought about that as well and actually wanted ServiceUsage to be an Optional, but since Optional is final this is not feasible (even if ServiceUsage would not be an interface).
Therefore my goal was to mimic Optional here with a similar get() method and isAvailable() (which also can be renamed to isPresent()).
But making it a Supplier that returns an Optional seems to be a bit inconvenient for me, because to eventually get the service instance one has to call serviceUsage.get().get() (if the service must be there), which I don't find nice.

But if, please don't use Exceptions to signal missing services, this gives very very bad error reporting, better use Optionals so the caller of that method can give good errors or even use defaults if a service is missing.

Is there another default that could be used than null? I think returning null is not desired, for the same reasons why one should use Optional over null. Furthermore I think the error message that is thrown by the ServiceUsage implementation is more descriptive than the one of Optional because it explicitly mentions the unavailable service.

[laeubi]

If you can return null or throw exception you can return an optional. Calling get() on Optional is actually not recommended, you should use orElse or orElseThrow to either handle or signal a missing item. Also the method name "get" is jsut the choice of supplier, as you implement an interface here you can always choose a better name like service or fetch as a method name.

[laeubi]

Actually I think ServiceCaller is a really bad idea as it recommends people using "simple" static service access where a proper service handling or better DI approach should be used. For example getting/ungetting a service can cause a lot of noise if you are using DS that might dispose service instances if they are no longer used.
Also the usage of getting the bundlecontext by classname can really cause some headache in the osgi-connect integration and I always assumed this as a last-resort than something you should use regularly.

So I personally not very convinced we should enhance ServiceCaller and use it more. But if, please don't use Exceptions to signal missing services, this gives very very bad error reporting, better use Optionals so the caller of that method can give good errors or even use defaults if a service is missing.

[HannesWell]

Actually I think ServiceCaller is a really bad idea as it recommends people using "simple" static service access where a proper service handling or better DI approach should be used. For example getting/ungetting a service can cause a lot of noise if you are using DS that might dispose service instances if they are no longer used.

Of course this is not the most performant way to get a service instance and it is definitely not a good idea to use that method inside a hot loop. The goal of this method is to provide a simple and save way to obtain a service instance in situations where for example OSGi DS or a ServiceTracker is not applicable or useful. I will clearly document that intend and the performance considerations.
However even if it can cause noise and could result in construction and destruction of a DS instance I would not expect a noticeable performance impact if the new method is only called occasionally. Of course it is more expensive than getting an instance of a existing ServiceTracker, but on the other hand instantiating a declarative service should also not be so expensive that one should avoid it as much as possible.

Furthermore the pattern to get/unget a service for a single use is already used in multiple more or less correct variations. And the suggested new method is IMHO a far more better way than for example what PDE (and IIRC P2 as well) does:
https://github.com/eclipse-pde/eclipse.pde/blob/3f9f33643122a8109460e88362593d67fde42ed5/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/PDECore.java#L401-L411

And there are more examples:

• https://github.com/eclipse-pde/eclipse.pde/search?q=ungetService
• https://github.com/eclipse-equinox/p2/search?q=ungetService

Also the usage of getting the bundlecontext by classname can really cause some headache in the osgi-connect integration and I always assumed this as a last-resort than something you should use regularly.

I'm fine in adding a overload with an additional BundleContext parameter. But why is using FrameworkUtil.getBundle(Class) a problem for OSGi-Connect? Isn't it the purpose of FrameworkUtilHelper to provide means to compute the Bundle of classes from 'connected' bundles?

@tjwatson or whoever is interested, can you please also give your general assessment about this proposal, too?
I would like to know if there is a general interest to add such method or if there are more objections before discussing the details.

[laeubi]

in situations where for example OSGi DS or a ServiceTracker is not applicable or useful.

DI (and its OSGi implementation DS) is always useful and/or applicable and ServiceTracker as well :-)

If I need to access a service statically and only once, then something is probably wrong. For example I then probably better get it passed as a parameter instead of struggling with it myself.

Furthermore the pattern to get/unget a service for a single use is already used in multiple more or less correct variations.

So these places are better refactored than adding another variant of a bad pattern.

Isn't it the purpose of FrameworkUtilHelper to provide means to compute the Bundle of classes from 'connected' bundles?

It is but that's just horrible complex because it requires once classloader = one bundle, while in connect many bundles can share the same classloader, or even have foreign classloaders. Aslo in almost all cases one don'T want to get the bundle but (as here as well) the bundle context and the javadoc of BundleContext clearly states:

BundleContext is generally for the private use of this bundle and is not meant to be shared with other bundles in the OSGi environment.

And as a bundle has always access to its bundle context (e.g. DS or Activators) there is really no need to use FrameworkUti at all (except for those bad usage patterns).

[tjwatson]

The opening comment gives this example code:

BundleContext ctx = FrameworkUtil.getBundle(Demo.class).getBundleContext();
ServiceReference<MyService> ref = ctx.getServiceReference(MyService.class);
MyService myService = ctx.getService(ref);
try {
	myService.doSomething();
} finally {
	ctx.ungetService(ref);
}

And it can be simplified to this with the proposal:

try (var service = ServiceCaller.useService(MyService.class)) {
	service.get().doSomething();
}

But the existing ServiceCaller already can simplify it to this with the use of callOnce:

ServiceCaller.callOnce(Demo.class, MyService.class, MyService::doSomething);

I'm not convinced a new method for calling a rarely used service is necessary.

[HannesWell]

But the existing ServiceCaller already can simplify it to this with the use of callOnce:

ServiceCaller.callOnce(Demo.class, MyService.class, MyService::doSomething);

I'm not convinced a new method for calling a rarely used service is necessary.

The opening example is indeed chosen badly in this regard.
The main advantage of the new method is when the invoked service method returns a result that should be returned in the caller and maybe also throws a checked exception. Given for example:

interface MyService<T> {
	String computeSomething() throws SomeCheckedException;
}

with the new method one can just call for example

try (var service = ServiceCaller.useService(MyService.class)) {
	return service.get().computeSomething();
} catch (SomeCheckedException e2) {
	// TODO: handle exception
}

or assign the return value to another variable and process it further outside the try-block.
Furthermore one can handle the exception by simply adding a catch-block to the try-block or just propagate it.
If the same should be done with the existing ServiceCaller.callOnce() it would probably look like this:

SomeCheckedException[] ex = new SomeCheckedException[1];
String[] result = new String[1];
ServiceCaller.callOnce(Demo.class, MyService.class, s -> {
	try {
		result[0] = s.computeSomething();
	} catch (SomeCheckedException e) {
		ex[0] = e;
	}
});
if (ex[0] != null) {
	// TODO: handle exception
}
return result[0];

Alternatively a ServiceCaller.callOnce() overload could be added that accepts a Function that can throw a generic Throwable and returns an Optional of the computed type:

public interface ThrowingFunction<T, R, E extends Throwable> {
	Optional<R> apply(T t) throws E;
}
public static <S, R, E extends Throwable> R callOnce(Class<?> caller, Class<S> serviceType, ThrowingFunction<S, R, E> function) throws E

This would indeed make simple computation a one liner if a possible exception should not be catched:

ServiceCaller.callOnce(Demo.class, MyService.class, MyService::computeSomething);

But if the exception should be catched a try-catch block has to be added again and it would IMHO be more readable with the new method.

Another advantage of the new approach is the ServiceUsage.all() method that provides a Stream of all registered service instances for a given service. That is not possible with ServiceCaller.callOnce() at the moment and would require another new method like callAllOnce().

[bjhargrave]

try (var service = ServiceCaller.useService(MyService.class)) {
	return service.get().computeSomething();
}

throws an exception if there is no service. This seems a bit harsh. I think you may be better off with an overload of the callonce method which is exception friendly.

[HannesWell]

After thinking about it a little more and considering the alternatives to add callOnce() overloads that accept exception friendly function, I think the best way is probably to keep the AutoCloseable approach but wrap the result of the get() call into a Optional as suggested by Christoph. But I renamed the get() method to first() to distinguish it from all() and to not have service.get().get() (if one is really sure the service is present).

If we would add more overloads that would lead to a combinatorics explosion if we want to provide means to obtain a Service with and without filter, from the caller class or a given context, want to use only the first or all serivces.
Furthermore the autocloseable approach allows to use multiple ServiceUsageScopes in one try block. Something similar could be achieved with nested callOnce() but the resulting code would probably be not so nice.
With the current proposal one could use it the following way:

public static class SomeCheckedException extends Exception {
}
interface MyService1 {
	void doSomething();
	String computeSometing();
}
interface MyService2 {
	String computeSomethingElse() throws SomeCheckedException;
}
...

try (var service1 = ServiceCaller.use(MyService1.class); //
	var service2 = ServiceCaller.use(MyService2.class)) {

	service1.first().ifPresent(MyService1::doSomething);
	String str = service2.first().get().computeSomethingElse();

	service1.all().forEach(MyService1::doSomething);
	List<String> strings1 = service1.all().map(MyService1::computeSometing).toList();

	List<String> strings2 = new ArrayList<>();
	for (MyService2 service : service2.all().toList()) {
		strings2.add(service.computeSomethingElse());
	}
} catch (SomeCheckedException e2) {
	// TODO: handle exception
}

The only real downside to the previous approach is that the Optional does not know the service name if it throws an exception in case of a absent service. Previously the NSE which was thrown by the get() method was more informative in this regard.
Besides that Optional offers much more possibilities than the previews approach.

[bjhargrave]

This result in unordered which means the all stream will report the ordered characteristic incorrectly and the first optional will hold something which is not the "first" as there is no ordering.

So you should sort the references in reverse order so the highest ranked service is first.

Suggested change

	ServiceReference<?>[] references = context.getServiceReferences(service.getName(), filter);
	ServiceReference<?>[] references = context.getServiceReferences(service.getName(), filter);
	Arrays.sort(references, Comparator.reverseOrder());

This means first will always be the highest ranked service and all will be ordered so that higher ranked services come before lower ranked services.

You may also want to rename first to highest.

[HannesWell]

This means first will always be the highest ranked service and all will be ordered so that higher ranked services come before lower ranked services.

That's a good suggestion, thank you.

But if I'm not missing anything, for example the result of the BundleContext.getServiceReference(Class) does not consider the service ranking does it?

You may also want to rename first to highest.

It would indeed by good to reflect that in the name, but I'm not so happy with the name highest(). But at the moment I don't have a better one.

[bjhargrave]

But if I'm not missing anything, for example the result of the BundleContext.getServiceReference(Class) does not consider the service ranking does it?

It does not. The order of the returned ServiceReferences is unspecified. So you need to sort them.

[bjhargrave]

The javadoc on these methods need more precision. first should state it returns the highest ranked service, if there are any services. all should state the stream is ordered in ranking order (highest ranked service first, lowest ranked service last).

[HannesWell]

Done.

[laeubi]

first can be default implemented as

Suggested change

	Optional<S> first();
	default Optional<S> first() {
	return all().findFirst();
	}

[bjhargrave]

True, but realistically there will only ever be one implementation of the interface so there is no real value in using a default method.

[tjwatson]

Should this be a public final class with a package accessible constructor so nothing else can construct it? That makes it clear there are no other implementations.

[HannesWell]

Should this be a public final class with a package accessible constructor so nothing else can construct it? That makes it clear there are no other implementations.

Done.

[HannesWell]

I updated the implementation and added another ServiceInstance class that allows a user to also query the properties of the service registrations. Besides that I also added documentation to all methods and nested classes and applied the suggested code-changes.

What's missing from my point, is a another usage example in the ServiceCaller javadoc and some tests. What do the others think?