Prevents potential NPE while closing resource

[gastaldi]

This prevents the following error from happening:

05:39:30 ERROR traceId=73a9ac0f0a387cf75ed794edd87e8576, parentId=, spanId=e48aff7d010659dd, sampled=true [io.qu.ve.ht.ru.QuarkusErrorHandler] (executor-thread-1) HTTP Request to /api/villains failed, error id: 315533e1-7ce0-4312-af88-2e1403e1cd4e-1: java.lang.NullPointerException: Cannot invoke "io.quarkus.bootstrap.runner.JarFileReference.close(io.quarkus.bootstrap.runner.JarResource)" because "ref" is null
	at io.quarkus.bootstrap.runner.JarResource.close(JarResource.java:158)
	at io.quarkus.bootstrap.runner.JarResource.resetInternalCaches(JarResource.java:165)
	at io.quarkus.bootstrap.runner.RunnerClassLoader.accessingResource(RunnerClassLoader.java:192)
	at io.quarkus.bootstrap.runner.RunnerClassLoader.loadClass(RunnerClassLoader.java:104)
	at io.quarkus.bootstrap.runner.RunnerClassLoader.loadClass(RunnerClassLoader.java:71)
	at io.vertx.core.http.impl.Http1xServerRequest.eventHandler(Http1xServerRequest.java:110)
	at io.vertx.core.http.impl.Http1xServerRequest.handler(Http1xServerRequest.java:314)
	at io.quarkus.vertx.http.runtime.ResumingRequestWrapper.handler(ResumingRequestWrapper.java:20)

• Introduced in Replace read/write lock in JarResource to avoid virtual threads pinning #40942
• See https://github.com/quarkusio/quarkus-super-heroes/actions/runs/10002970451/job/27649141577

[geoand]

I'd rather have @mariofusco look at this one since he has been working in this area lately.

That said, the change does seem to make sense

[mariofusco]

@gastaldi To be honest I put there that assertion exactly because I didn't expect that reference to be null and I wanted a fail fast mechanism in this case instead of hiding the dust under the carpet with a null check.

@geoand @gsmet In essence if we're in a hurry it is ok for me to merge this pull request (at least as a temporary solution), but I'm afraid that this is fixing the symptom of the problem rather than its root cause and I will need to investigate it further regardless. I will try to reproduce the problem on my side (any hint on how to do this will be welcome) and I will keep you updated with my findings.

[gastaldi]

@mariofusco the problem is that the AssertionError is never thrown there (probably because it doesn't run with assertions enabled), so we can't rely on the assert keyword really.

A more conservative approach would be to revert #40942 and merge it back when we figure out what is causing that reference to be null

[geoand]

but I'm afraid that this is fixing the symptom of the problem rather than its root cause

That's why I wanted your input 😉

[gsmet]

Fwiw I haven’t modified the runner CL.

As for being in a hurry, if this code is in the 3.13 branch, I don’t feel comfortable pushing 3.13.0 with a possible NPE.

So either we find the root cause by Tuesday evening or I would recommend to merge (and backport this).

I’m on my phone so can’t easily check if this is in 3.13.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit bc1a361.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging/deployment

✖ io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

• Expecting actual: ["-4","-5","-6","-7","-8","-9","-10","-11"] to start with: ["-3", "-4", "-5", "-6"] - java.lang.AssertionError

java.lang.AssertionError: 

Expecting actual:
  ["-4","-5","-6","-7","-8","-9","-10","-11"]
to start with:
  ["-3", "-4", "-5", "-6"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:36)

[gsmet]

I will merge this to get it in 3.13.0.

Please follow up on this when you have the time to investigate and add the backport label so that we push it to 3.13.

[mariofusco]

I spent a few hours trying to reproduce this NPE, but with no luck. I understand that it could eventually be caused by a seldomly happening race condition, but I ran the test-suite of that rest-villains project hundreds of times against the latest quarkus snapshot, from both the IDE and the CLI, and I've never seen it happening.

Meanwhile I tried to also reread and statically analyze my code going line by line, but I really don't see how that reference could be possibly null at that point. That CompletableFuture is published as already completed, and once completed its value cannot be changed anymore, so either the AtomicReference is empty (and there is a already a null check for that case) or it contains a future pointing to a JarResource.

The only other possible alternative, and actually the only situation where that CompletableFuture couldn't have a value, is when it is not possible at all to load the requested jarFile. This could maybe explain why I couldn't reproduce that problem locally, but even in this case I'd expect some other failure to happen far before reaching the close() (like a ClassNotFoundException), or at least to have that IOException in the continuous integration log, but I don't see any track of it.

That said, I'm still unsure why this further null check could be necessary, but at this point I don't have anything against it. Sorry for taking some time, but I just want to be 100% sure that I wasn't overlooking anything in the implementation of the class loader.

[gsmet]

Oh no worries, I didn't want to press you, quite the opposite actually. I merged it so that you have more time to figure it out if you needed to.
I'm very open to a follow up if at some point we detect an issue.

Thanks for the investigation!

[mariofusco]

I will merge this to get it in 3.13.0.

Please follow up on this when you have the time to investigate and add the backport label so that we push it to 3.13.

@gsmet Thanks for merging this, bad timing on my side, I published my feedback in the moment you hit the merge button :)

I did all possible investigations and as you can read in my former comment it's ok for me to merge this pull request even though I dind't find a reasonable explanation for that NPE, neither I've been able to reproduce it in any way.