Fix jar file reference close race condition

[franz1981]

Possible solution for #43158

This is fixing a potential race while virtual threads try to "help" and in-progress release by moving forward the shared atomic reference: we need to make sure that JarFileReferences cleanup don't mess up with a freshly jar file shared by the virtual thread.
At the same time if there's an in progress jar file ref load from a platform thread, the virtual thread doesn't have to replace the in progress task.

[franz1981]

FYI @pcasaes if you can give it a shot: I've implemented this quickly and using the simplest scheme i.e. using the flip into negative value to bound the domain of reference count values.

It has 2 strict requirements:

1. acquire and releases should always be in pair
2. releases can never close the file if the reference hasn't been "marked for closing"

I hope I'm not making it too strict

[franz1981]

@mariofusco PTAL

@dmlloyd too

If you prefer me to use a parity bit or any other mechanism to simplify the reasoning here, any suggestion is welcome

[franz1981]

I've found another logic error in the original implementation for the virtual threading part, fixing it

[franz1981]

@mariofusco @geoand

Re

quarkus/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/JarResource.java

Lines 152 to 160 in 31fc642

	var futureRef = jarFileReference.get();
	if (futureRef != null) {
	// The jarfile has been already used and it's going to be removed from the cache,
	// so the future must be already completed
	var ref = futureRef.getNow(null);
	if (ref != null) {
	ref.markForClosing(this);
	}
	}

I'm concerned re this because if markForClosing is mandatory to close the jar file, it means that if we have a race between this close and a loading which doesn't have yet completed the future, it will miss it - causing the file ref to not be closed: this is fine/expected?

[franz1981]

@gsmet I have to check yet if we need some tests and counters to verify that after a reset of caches all the jar file ref are correctly nulled out or that a retained jar file ref (acquired by some thread, and not yet released) - after a reset of caches - it's correctly closed (and nulled out) after being released.

[quarkus-bot]

Status for workflow Quarkus CI

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

✅ 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

⚙️ Native Tests - HTTP

📦 integration-tests/rest-client-reactive

✖ io.quarkus.it.rest.client.BasicTestIT.shouldCreateClientSpans - History

• expected: <1> but was: <2> - org.opentest4j.AssertionFailedError

org.opentest4j.AssertionFailedError: expected: <1> but was: <2>
	at io.quarkus.it.rest.client.BasicTest.shouldCreateClientSpans(BasicTest.java:216)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestMethod(QuarkusTestExtension.java:810)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

[mariofusco]

This method doesn't really close the resource, but mark it as ready to be closed, now its method better reflect this behaviour.

[geoand]

This comment should be in the code, not here :)

[mariofusco]

The referenceCounter is turned into a negative value to indicate (in an idempotent way) that the resource has been marked to be closed.

[geoand]

Same as above

[mariofusco]

The acquire failure implies that the reference is already marked to be closed.

[geoand]

Again

[mariofusco]

The former implementation was designed under the strong assumption that the close method on the JarFileReference was called once and only once. This issue demonstrates that this assumption was no longer true, so the main goal of this pull request is making the close idempotent (or more precisely the markForClosing since this is semantic of that method).

[mariofusco]

@gsmet @geoand This fix has been also already verified by @pcasaes, so I believe that this pull request is ready to be merged.

[gsmet]

Let's merge this to be able to backport it to 3.14.4.

@mariofusco As for the comments, please add them in a follow-up PR, that we will merge and backport later.

[gsmet]

Thanks for taking a look at this.

[pcasaes]

Nice! Thanks a lot!