Avoid calling ServiceLoader twice with same class loader

[gsmet]

Motivation:

The ServiceLoader goes through all the jars to find its files and it seems like a good idea to avoid doing it twice for the same class loader.

This is somehow related to the work we are doing in Quarkus for Leyden.

Conformance:

You should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

I think I have signed the ECA already to work on Bean Validation.

[gsmet]

FYI @geoand

[gsmet]

I'm interested in having this backported to 4.x for current Quarkus.

[gsmet]

BTW, just to clarify: in my case, it was for a service file that didn't exist so we were going through all the calls as the list was empty.

[tsegismont]

Thanks @gsmet

I pushed a commit to simplify the method for readability. Tell me what you think

[tsegismont]

I'm interested in having this backported to 4.x for current Quarkus.

@gsmet fine with me. When this one is merged, can you send PRs to the 4.x and 5.0 branches please? Thanks

Please note that, in Vert.x 5, this class has moved to an impl package (not visible in a modular Java app)

[gsmet]

So it all depends on what you want to do. My initial PR didn't change the logic.

Your commit changes the logic right? If you enforce a CL, we don't try to fallback to the ServiceHelper CL anymore? Also didn't you drop the condition that is the very reason of my PR in the TCCL case? Avoid interrogating the same CL twice?

[tsegismont]

So it all depends on what you want to do. My initial PR didn't change the logic.

Right

Your commit changes the logic right? If you enforce a CL, we don't try to fallback to the ServiceHelper CL anymore? Also didn't you drop the condition that is the very reason of my PR in the TCCL case? Avoid interrogating the same CL twice?

Yes, it does. I think we messed-up somewhere, and started to try loading factories twice even when the user provided a specific classloader.

As the comment at the bottom suggests, we should try again only when the first attempt was with the TCCL.

The original change is #1058

[gsmet]

But the TCCL might be what loaded ServiceHelper so I'm not sure why you dropped my optimization in the TCCL case.

[tsegismont]

If you invoke io.vertx.core.impl.ServiceHelper#loadFactories(java.lang.Class<T>, java.lang.ClassLoader) by providing a classloader instead of null, there will be a single attempt.

[gsmet]

Yeah, I understand that. But why make the TCCL case worse than with my change? I don't understand the rationale?

[gsmet]

I'm specifically talking about having && TCCL != ServiceHelper.class.getClassLoader() in the condition before falling back.

[vietj]

is that possible to get a test for this ?

[tsegismont]

As discussed off-list, the code in master right now tries loading factories twice, even when a classloader is provided, hence the confusion.

The original intent of this PR is to avoid loading factories twice when the TCCL happens to be the same as the ServiceHelper's classloader.

[tsegismont]

is that possible to get a test for this ?

If we had a mocking lib in the test scope, we could verify io.vertx.core.impl.ServiceHelper#loadFactories(java.util.ServiceLoader<T>) is invoked exactly once when:

• io.vertx.core.impl.ServiceHelper#loadFactories(java.lang.Class<T>, java.lang.ClassLoader) is invoked with classloader set to null
• the TCCL is set to ServiceHelper.class.getClassLoader()

Otherwise, I don't know how we can check what our code does with the classloader

[vietj]

I think we already have structure for this to verify loading with a classloader

[tsegismont]

I think we already have structure for this to verify loading with a classloader

I see that in ServiceHelperTest, thanks.

@gsmet can you share a stacktrace or flamegraph that shows how the classloader was searched several times so that we can intercept the right methods and verify assumptions?