Make jib-core a parent-first dependency

[geoand]

Done in order to avoid problems with Jib's use of
lock classes which have no effect when the lock
class is loaded from different ClassLoaders

Fixes: #11903

[famod]

FTR, I have not forgotten about this but other things got/are getting in the way.

[geoand]

No worries, whenever you have time

[famod]

I'm sorry but I have to report that this change is not fixing the OverlappingFileLockException in a multi-threaded Maven build.

I might be totally wrong here but AFAICS, if there is not also a "vanilla" lock in jib (synchronized or some java.util.concurrent.locks.Lock), then I don't see how limiting the classloaders involved can prevent OverlappingFileLockException in the first place.
E.g. you should be able to provoke this issue in a totally flat classloader hierarchy (or single CL) just by kicking off two threads that try to lock the same file via FileChannel.lock().
If my assumption is right, then either jib should add a "vanilla" lock upstream or Quarkus could add one around the jib invocation. The latter would be less efficient/more coarse grained as the entire jib execution would run inside the lock.

[geoand]

I didn't really look at what the Jib code does, I just assumed that it was using a class lock, hence the proposed change.

In any case, thanks for checking, I'll close this and we should probably implement your suggestion about running our Jib code in a lock (definitely not great, but I don't see what else we can do...)

[famod]

Ha, they do have a lock already: https://github.com/GoogleContainerTools/jib/blob/cbf7ae9591e0103c6dd6bd249a6b3c0e1dad9329/jib-core/src/main/java/com/google/cloud/tools/jib/filesystem/LockFile.java#L55

So I'm wondering why your adjustment didn't work...

[famod]

Btw, I'm currently testing a very primitive synchronized in JibProcessor.

PS: Doesn't work, which is not that surprising.

[famod]

Even with parent-first, I can see that com.google.cloud.tools.jib.api.JibContainer is loaded in three different QuarkusClassLoader:Augmentation Class Loader: PROD@* CLs (with 3 concurrent Maven modules building a Quarkus app via jib). Their parent CL is the same though.

[geoand]

That shouldn't happen, I'll have to actually run it.

Do you have a sample multi-module application I can use?

[famod]

I'm afraid no, I'm testing with my actual corporate & closed-source project.

[geoand]

🆗

[famod]

@geoand got something for you: q_jib-parallel.zip
Run it in a loop:
time while mvn -q package -T2 -Dquarkus.container-image.build=true; do echo ----; rm -rf /tmp/jib-core-application-layers-cache; done

Should fail very quickly.

PS: I had to add that cache cleanup while testing my actual app build in the same kind of loop because after a while I got "no space left on device" (Ubuntu 22). Looks like another issue to me.

[geoand]

Nice!

[geoand]

So the reason why my parent-first change didn't do anything is because parentFirst artifacts are for the runtime classloaders, not the build time.

[geoand]

Btw, I'm currently testing a very primitive synchronized in JibProcessor.

How did this go?

[famod]

It didn't work for the same classloading reasons.
I used JibContainer.class as the lock object though. I think it should work when locking on the parent classloader, but that seems kind of dirty?

[geoand]

I used JibContainer.class as the lock object though. I think it should work when locking on the parent classloader, but that seems kind of dirty?

Yeah, that would, but so would just using any JDK class :)

[famod]

@geoand In case we'd find a "suitable" Quarkus class to lock (that was loaded by the common CL), would that be an acceptable workaround for you?

[geoand]

Sure, but given that Quarkus ClassLoaders are created for each Maven build, I don't see how that could work.

[famod]

@geoand this quick hack in JibProcessor works:

Object lockObj;
try {
    lockObj = getClass().getClassLoader().getParent().loadClass("io.quarkus.deployment.GlobalLock");
} catch (Throwable t) {
    throw new RuntimeException(t);
}
JibContainer container;
synchronized (lockObj) {
    container = jibContainerBuilder.containerize(containerizer);
}

This GlobalLock (preliminary naming) is located in core-deployment and it could provide convenience methods to hide this ugly classloading detour and to support multiple different locks, not only for Jib (though I hope it is and will stay the only case). It probably also needs some safeguards and checks as well.

Not pretty at all, but WDYT?

PS: Someone would need to check Gradle, too.

[geoand]

Yeah, I think we can live with that hack.

I'd rather not have any util methods though and keep this only to Jib for now and hope we never need it anywhere else

[geoand]

Was going to ask about Gradle, but I see you already did 😁

[famod]

I'd rather not have any util methods though and keep this only to Jib for now and hope we never need it anywhere else

Ok, but how? The parent CL doesn't know about the jib-deployment module. That was my first attempt, btw. 😉

[geoand]

I'd just use (semi)some random class in the deployment module

[famod]

@geoand

I'd just use (semi)some random class in the deployment module

You mean core-deployment, right?

[geoand]

Correct