Check if bundle contexes correctly unget services after invoking getService

[LucaDazi]

Several OSGi Services are acquired using the bundle context.
After the services are no more needed they should be released calling ungetService(...) in order to keep the reference count of said services consistant.

[cdealti]

@LucaDazi do you remember what was the conclusion?

[cdealti]

@ctron can you please take a look? I noticed you fixed something related in #530.
What are the implications of not ungetting a service?

[ctron]

Yes #530 is exactly that.

Beside violating the spec 😉 …

Looking at Equinox's implementation this can happen:

if (useCount == Integer.MAX_VALUE) {
   throw new ServiceException(Msg.SERVICE_USE_OVERFLOW);
}

Now MAX_VALUE is quite a number, but running for some time, constantly getting called, you eventually reach it.

For ServiceFactory objects, they won't get garbage collected.

You probably won't run into any memory leaks, since everything seems to be reference counted.

But then again, cleaning up afterwards and following the spec seems like the right thing to do.

[cdealti]

@ctron are you willing to find and fix all the unbalanced occurrences for Kura 3.0?

[ctron]

Not really.

[cdealti]

@nicolatimeus can you please find all occurrences of these unbalanced calls and provide a list of the unbalanced calls for estimating the effort?

[nicolatimeus]

The table below reports the the invocations to BundleContext.getService().

Call Location	Released on all normal control paths	Always released on exception
ShellScriptResourceProcessorImpl.java:72	yes	yes
ServiceLocator.java:112	yes	yes
AssetServiceImpl.java:48	yes, but some references might be ungetted even if not needed
BluetoothDeviceImpl.java:60	no	no
GwtWireServiceImpl.java:154	no	no
DriverServiceImpl.java:49	yes, but some references might be ungetted even if not needed
ComponentUtil.java:68	yes	yes
ComponentUtil.java:138	no	no
ComponentUtil.java:176	no	no
ComponentUtil.java:424	no	no
WireHelperServiceImpl.java:75	yes, but some references might be ungetted even if not needed
ConfigurationServiceImpl.java:1080	yes	yes
ServiceLocator.java:191	no	no
WireHelperServiceImpl.java:112	yes, but some references might be ungetted even if not needed
GwtWireServiceUtil.java:123	no	no
GwtWireServiceUtil.java:161	yes	no
GpsClockSyncProvider.java:90	no	no
GpsClockSyncProvider.java:87	no	no
WireHelperServiceImpl.java:132	yes, but some references might be ungetted even if not needed
WireHelperServiceImpl.java:151	yes, but some references might be ungetted even if not needed
LinuxProcessUtil.java:411	yes	no
AssetServiceImpl.java:84	reference is released even if service is still in use
DriverServiceImpl.java:85	reference is released even if service is still in use
XmlConfigPropertiesAdapter.java:85	no	no
ExamplePublisher.java:58	no	no
ExamplePublisher.java:70	no	no
CloudSubscriber.java:90	no	no
CloudServiceImpl.java:459	no	no
ConfigurableComponentTracker.java:80	yes	yes
WireComponentTrackerCustomizer.java:172	yes	no
XmlConfigPropertiesAdapter.java:258	no	no
ServiceLocator.java:174	yes	yes

The ServiceLocator.getService(Class), ServiceLocator.getServices(Class), and ServiceLocator.getServices(Class, String) methods in the web ui bundle obtain a service reference to the requested service/s, calls BundleContext.getService() on it and return the result.
Using these methods will probably always result in an unbalanced reference count, unless the caller obtains another service reference to the returned service and call BundleContext.ungetService() on it.
These methods should probably be redesigned.

The table below report the number of call sites for each method according to Eclipse:

Method	Number of call sites
ServiceLocator.getService(Class)	>60
ServiceLocator.getServices(Class)	0
ServiceLocator.getServices(Class, String)	2

The table below reports the details about ServiceLocator.getService(ServiceReference), which obtains a BundleContext instance, calls BundleContext.getService() on it and then return the result.

Call Location	Released on all normal control paths	Always released on exception
GwtStatusServiceImpl.java:89	yes	no
GwtCloudServiceImpl.java:202	yes	yes
GwtCloudServiceImpl.java:229	yes	yes
GwtStatusServiceImpl.java:117	yes	no
GwtStatusServiceImpl.java:169	yes	yes
GwtStatusServiceImpl.java:190	yes	yes
GwtCloudServiceImpl.java:144	yes	yes
GwtCloudServiceImpl.java:270	no	no
GwtCloudServiceImpl.java:250	no	no
ServiceLocator.java:72	probably not (see above)
ServiceLocator.java:213	probably not (see above)

[cdealti]

There's some duplication in [1] and [2].

@ctron @amitjoy wouldn't be better to collapse everything in the org.eclipse.kura.util bundle?

[1] https://github.com/eclipse/kura/blob/develop/kura/org.eclipse.kura.util/src/main/java/org/eclipse/kura/util/service/ServiceUtil.java
[2] https://github.com/eclipse/kura/blob/develop/kura/org.eclipse.kura.web2/src/main/java/org/eclipse/kura/web/server/util/ServiceLocator.java#L174

[amitjoy]

@cdealti I second that 👍

[ctron]

Sure, go ahead.

[cdealti]

@amitjoy are you willing to do that ;-)?

[amitjoy]

Just had a quick look on the findings and I agree that in OSGi, the low-level methods can easily be misused by any developer and that's why we had a long discussion in #797. To cope up with the requirement of providing an easy to use helper class, I have designed an utility class in #1303. If it gets merged, we can refactor the aforementioned classes accordingly. I would be happy to do that.

[github-actions]

This issue is stale because it has been open for 60 days with no activity.

[github-actions]

This issue was closed because it has been inactive for 14 days since being marked as stale.