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?

[gsmet]

Typically, you have this case:

[gsmet]

Note that in this particular run, the TCCL was different from the ServiceHelper class loader and in this case, we will still try to load files twice, unfortunately.

With fast-jar, the optimization should work, with what we have to use for AOT, it won't as we have to load the classes from JDK class loader for them to be cached.

Let me clarify a bit:

• fast-jar: TCCL is also the CL loading most of the application classes
• aot-jar: TCCL is just a thin layer to load resources (including service files) and the loading of classes themselves has to be delegated to the JDK class loader for the AOT cache class loading to work

I'm wondering if having all frameworks to pay the price for OSGi might be a bit counter productive but these issues are hard to solve :/.

[tsegismont]

Thanks for the detailed answer @gsmet

@vietj @gsmet review the last commit that provides a test

[gsmet]

I was thinking a bit more about it and I was wondering if we shouldn’t have the option to pass a CL to this various ServiceLoader calls when initializing the VertxBuilder.

Because in our case, we would like to enforce the CL and always avoid this two calls.

Doesn’t prevent us to merge this PR but I wonder if we should push it a bit further so that we are sure we never do the dual calls with Quarkus.