Should not cache the ServiceProvider implementation

[desruisseaux]

When ServiceProvider.current() or available() is invoked for the first time, these methods search for ServiceProvider implementations using the following:

ServiceLoader.load(ServiceProvider.class)

The result of above search should not be cached in ServiceProvider.current field. More specifically, the following lines should be removed or edited:

https://github.com/unitsofmeasurement/unit-api/blob/b5ca1576b7df480d7692ea26bbfd9c332945b5c0/src/main/java/javax/measure/spi/ServiceProvider.java#L305

https://github.com/unitsofmeasurement/unit-api/blob/b5ca1576b7df480d7692ea26bbfd9c332945b5c0/src/main/java/javax/measure/spi/ServiceProvider.java#L387

The reason is explained in the JDK 18 Javadoc:

Service loader objects obtained with this method should not be cached VM-wide. For example, different applications in the same VM may have different thread context class loaders. A lookup by one application may locate a service provider that is only visible via its thread context class loader and so is not suitable to be located by the other application. Memory leaks can also arise. A thread local may be suited to some applications.

I faced this problems during the development of Geospatial Integrity of Geoscience Software (GIGS) tests. Consider the following scenario, where each box is a set of classes loaded with a different class loader:

Because of the caching done in ServiceProvider, if implementation A is executed first, implementation B will see the JSR-385 implementation of A. In some situations (I have not explored the details), ServiceProvider.current() see no implementation at all even if implementations A or B is on the classpath, because the context ClassLoader at ServiceProvider.current() invocation time is the one of the yellow box on the top, which does not contain any implementation.

In summary, advanced applications need to specify the ClassLoader explicitly, but current ServiceProvider static methods do not allow that. Consequently advanced applications need to use ServiceLoader themselves. This is okay, but the caching done by ServiceProvider is a problem. I suggest to remove this caching and I can create a pull request, but because it would be a behaviour change I guess it needs to be discussed first.

[desruisseaux]

Implemented the removing of the cache in https://github.com/Geomatys/unit-api/commit/fa97d580990ef38a53ad6cda21b6e1e273d211b9

The current private field stay present because it is still needed by setCurrent(ServiceProvider) method. I would be in favour of deprecating that method, but that would be another discussion.

I have not yet created Pull Request for this commit, waiting to see if there is discussion first.

[keilw]

@desruisseaux Is there a strong use case having multiple implementations in the same VM?

Did you also have a look at indriya#383, which is not directly related (because NumberSystem is not declared by the API at this point) but it relates to different class loaders and setCurrentNumberSystem() was modeled after it?

What are possible performance penalties, did you test that in the Geomatys PR?

setDefault() is private, so there is not a big problem to change it or even remove the method completely, but setCurrent() is part of the public API/SPI, so while it seems quite seldomly used (even the extensions I know/created do not use it right now) so we must not simply replace it unless there's at least one or even two further MRs of JSR 385. Let's check with PMO but I suspect even deprecating it first needs another MR while an internal rewiring could be done in a simple Service Update.

As long as the of() method still allows to access a named ServiceProvider of choice the long term impact of removing setCurrent() seems manageable, but we have to go the administrative way.

[desruisseaux]

Hello

Is there a strong use case having multiple implementations in the same VM?

Mostly containers (for example Tomcat) which can host an arbitrary amount of applications. Above-cited GIGS tests are also a kind of container. Those containers isolate each application by giving them their own ClassLoader. The use of a shared cache breaks this isolation.

I was not aware of https://github.com/unitsofmeasurement/indriya/issues/383 but I believe that this fundamentally the same issue than this one. The fix that they propose (adding a ClassLoader parameter) could also apply ServiceProvider. But that would be a separated issue.

What are possible performance penalties, did you test that in the Geomatys PR?

The performance penalties depends on how the application uses ServiceProvider.current(). In my applications, that method is invoked only once and cached by the application (this is okay if the cache is done by the application itself, not by ServiceProvider, because in such case it applies only to that particular application, not to all applications on the containers). So the change has no impact. However if other applications invokes ServiceProvider.current() frequently, then a performance degradation is to be expected, but we have not measured by how much.

so we must not simply replace [ServiceProvider.setCurrent(…)] unless there's at least one or even two further MRs of JSR 385. Let's check with PMO but I suspect even deprecating it first needs another MR while an internal rewiring could be done in a simple Service Update.

Yes this issue is not urgent and I'm fine with waiting for more MR. I agree with above approach, and indeed the proposed change in Geomatys repository does not touch to the ServiceProvider.setCurrent(…) method. We may open a separated issue for that.

[keilw]

Is there a chance this could also improve the code coverage, see #219?

[desruisseaux]

It may slightly improve code coverage as a side-effect of the fact that the proposed commit removes some lines of code. But this is not really the way we are supposed to improve code coverage. We would need to add a test case.

Adding a unit test may be a little bit easier with the removal of the cache. Because we can use a dummy service for testing purpose, and make sure that the dummy service does not pollute other tests (does not stay in the cache during execution of other tests).

[keilw]

@desruisseaux Do you see a chance to work on this one soon? We do not have a totally strict date, but the idea was to get MR2 of JSR 385 out some time around World Metrology Day 2023 (May 20), at least the code release. If the review and ballot were to take a bit beyond that, no problem,ideally we'd also get those done before the Summer break. If you see a huge problem with this, we could always propose another MR down the road, e.g. JSR 269 had at least one per year since 2018.

[desruisseaux]

Hello @keilw . This is pull request #246 created a few months ago. Is there additional work needed, apart JUnit tests? Regarding JUnit, adding a test case would be difficult. Actually, for now I don't see well how to test with JUnit. Testing would require a separated application which reproduce the graphs shown in the description of this issue.

[desruisseaux]

Closing since #246 has been merged.