From cfe603048a1229dd409724f1dd236504193e88f3 Mon Sep 17 00:00:00 2001 From: mariofusco Date: Mon, 3 Jun 2024 17:22:07 +0200 Subject: [PATCH] Replace read/write lock in JarResource to avoid virtual threads pinning --- .../bootstrap/runner/JarFileReference.java | 143 ++++++++++++++++++ .../quarkus/bootstrap/runner/JarResource.java | 131 +++++----------- 2 files changed, 182 insertions(+), 92 deletions(-) create mode 100644 independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/JarFileReference.java diff --git a/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/JarFileReference.java b/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/JarFileReference.java new file mode 100644 index 0000000000000..766b2cdd6ad68 --- /dev/null +++ b/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/JarFileReference.java @@ -0,0 +1,143 @@ +package io.quarkus.bootstrap.runner; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; +import java.util.jar.JarFile; + +import io.smallrye.common.io.jar.JarFiles; + +public class JarFileReference { + // Guarded by an atomic reader counter that emulate the behaviour of a read/write lock. + // To enable virtual threads compatibility and avoid pinning it is not possible to use an explicit read/write lock + // because the jarFile access may happen inside a native call (for example triggered by the RunnerClassLoader) + // and then it is necessary to avoid blocking on it. + private final JarFile jarFile; + + private final AtomicReference jarFileReference; + + // The referenceCounter - 1 represents the number of effective readers (#aqcuire - #release), while the first + // reference is used to determine if a close has been required. + // The referenceCounter starts from 2 because a create always implies also a first acquire. + private final AtomicInteger referenceCounter = new AtomicInteger(2); + + private JarFileReference(AtomicReference jarFileReference, JarFile jarFile) { + this.jarFileReference = jarFileReference; + this.jarFile = jarFile; + } + + /** + * Increase the readers counter of the jarFile. + * + * @return true if the acquire succeeded: it's now safe to access and use the inner jarFile. + * false if the jar reference is going to be closed and then no longer usable. + */ + private boolean acquire() { + while (true) { + int count = referenceCounter.get(); + if (count == 0) { + return false; + } + if (referenceCounter.compareAndSet(count, count + 1)) { + return true; + } + } + } + + /** + * Decrease the readers counter of the jarFile. + * If the counter drops to 0 and a release has been requested also closes the jarFile. + * + * @return true if the release also closes the underlying jarFile. + */ + private boolean release() { + while (true) { + int count = referenceCounter.get(); + if (count <= 0) { + throw new IllegalStateException( + "The reference counter cannot be negative, found: " + (referenceCounter.get() - 1)); + } + if (referenceCounter.compareAndSet(count, count - 1)) { + if (count == 1) { + try { + jarFile.close(); + } catch (IOException e) { + // ignore + } finally { + jarFileReference.compareAndSet(this, null); + } + return true; + } + return false; + } + } + } + + /** + * Ask to close this reference. + * If there are no readers currently accessing the jarFile also close it, otherwise defer the closing when the last reader + * will leave. + */ + void close() { + release(); + } + + @FunctionalInterface + interface JarFileConsumer { + T apply(JarFile jarFile, Path jarPath, String resource); + } + + static T withJarFile(AtomicReference jarFileReference, Path jarPath, String resource, + JarFileConsumer fileConsumer) { + + // Happy path: the jar reference already exists and it's ready to be used + final var localJarFileRef = jarFileReference.get(); + if (localJarFileRef != null && localJarFileRef.acquire()) { + return consumeSharedJarFile(jarPath, resource, fileConsumer, localJarFileRef); + } + + // There's no valid jar reference, so load a new one + final var newJarFileRef = JarFileReference.loadAcquiredJarFile(jarFileReference, jarPath); + if (jarFileReference.compareAndSet(localJarFileRef, newJarFileRef) || + jarFileReference.compareAndSet(null, newJarFileRef)) { + // The new file reference has been successfully published and can be used by the current and other threads + return consumeSharedJarFile(jarPath, resource, fileConsumer, newJarFileRef); + } + + // The newly created file reference hasn't been published so it can be used exclusively by the current thread + return consumeUnsharedJarFile(jarFileReference, jarPath, resource, fileConsumer, newJarFileRef); + } + + private static T consumeSharedJarFile(Path jarPath, String resource, JarFileConsumer fileConsumer, + JarFileReference jarFileRef) { + try { + return fileConsumer.apply(jarFileRef.jarFile, jarPath, resource); + } finally { + jarFileRef.release(); + } + } + + private static T consumeUnsharedJarFile(AtomicReference jarFileReference, Path jarPath, + String resource, JarFileConsumer fileConsumer, JarFileReference jarFileRef) { + try { + return fileConsumer.apply(jarFileRef.jarFile, jarPath, resource); + } finally { + boolean closed = jarFileRef.release(); + assert !closed; + // Check one last time if the file reference can be published and reused by other threads, otherwise close it + if (!jarFileReference.compareAndSet(null, jarFileRef)) { + closed = jarFileRef.release(); + assert closed; + } + } + } + + private static JarFileReference loadAcquiredJarFile(AtomicReference jarFileReference, Path jarPath) { + try { + return new JarFileReference(jarFileReference, JarFiles.create(jarPath.toFile())); + } catch (IOException e) { + throw new RuntimeException("Failed to open " + jarPath, e); + } + } +} diff --git a/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/JarResource.java b/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/JarResource.java index 8b4da096a891d..85beedba4fbaf 100644 --- a/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/JarResource.java +++ b/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/JarResource.java @@ -11,16 +11,12 @@ import java.security.ProtectionDomain; import java.security.cert.Certificate; import java.util.Objects; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReadWriteLock; -import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.concurrent.atomic.AtomicReference; import java.util.jar.JarEntry; import java.util.jar.JarFile; import java.util.zip.ZipEntry; -import java.util.zip.ZipFile; import io.smallrye.common.io.jar.JarEntries; -import io.smallrye.common.io.jar.JarFiles; /** * A jar resource @@ -30,25 +26,13 @@ public class JarResource implements ClassLoadingResource { private final ManifestInfo manifestInfo; private final Path jarPath; - private final Lock readLock; - private final Lock writeLock; - private volatile ProtectionDomain protectionDomain; - //Guarded by the read/write lock; open/close operations on the JarFile require the exclusive lock, - //while using an existing open reference can use the shared lock. - //If a lock is acquired, and as long as it's owned, we ensure that the zipFile reference - //points to an open JarFile instance, and read operations are valid. - //To close the jar, the exclusive lock must be owned, and reference will be set to null before releasing it. - //Likewise, opening a JarFile requires the exclusive lock. - private volatile JarFile zipFile; + private final AtomicReference jarFileReference = new AtomicReference<>(); public JarResource(ManifestInfo manifestInfo, Path jarPath) { this.manifestInfo = manifestInfo; this.jarPath = jarPath; - final ReadWriteLock readWriteLock = new ReentrantReadWriteLock(); - this.readLock = readWriteLock.readLock(); - this.writeLock = readWriteLock.writeLock(); } @Override @@ -69,38 +53,48 @@ public void init() { @Override public byte[] getResourceData(String resource) { - final ZipFile zipFile = readLockAcquireAndGetJarReference(); - try { - ZipEntry entry = zipFile.getEntry(resource); + return JarFileReference.withJarFile(jarFileReference, jarPath, resource, JarResourceDataProvider.INSTANCE); + } + + private static class JarResourceDataProvider implements JarFileReference.JarFileConsumer { + private static final JarResourceDataProvider INSTANCE = new JarResourceDataProvider(); + + @Override + public byte[] apply(JarFile jarFile, Path path, String res) { + ZipEntry entry = jarFile.getEntry(res); if (entry == null) { return null; } - try (InputStream is = zipFile.getInputStream(entry)) { + try (InputStream is = jarFile.getInputStream(entry)) { byte[] data = new byte[(int) entry.getSize()]; int pos = 0; int rem = data.length; while (rem > 0) { int read = is.read(data, pos, rem); if (read == -1) { - throw new RuntimeException("Failed to read all data for " + resource); + throw new RuntimeException("Failed to read all data for " + res); } pos += read; rem -= read; } return data; } catch (IOException e) { - throw new RuntimeException("Failed to read zip entry " + resource, e); + throw new RuntimeException("Failed to read zip entry " + res, e); } - } finally { - readLock.unlock(); } } @Override public URL getResourceURL(String resource) { - final JarFile jarFile = readLockAcquireAndGetJarReference(); - try { - JarEntry entry = jarFile.getJarEntry(resource); + return JarFileReference.withJarFile(jarFileReference, jarPath, resource, JarResourceURLProvider.INSTANCE); + } + + private static class JarResourceURLProvider implements JarFileReference.JarFileConsumer { + private static final JarResourceURLProvider INSTANCE = new JarResourceURLProvider(); + + @Override + public URL apply(JarFile jarFile, Path path, String res) { + JarEntry entry = jarFile.getJarEntry(res); if (entry == null) { return null; } @@ -110,15 +104,7 @@ public URL getResourceURL(String resource) { if (realName.endsWith("/")) { realName = realName.substring(0, realName.length() - 1); } - final URI jarUri = jarPath.toUri(); - // first create a URI which includes both the jar file path and the relative resource name - // and then invoke a toURL on it. The URI reconstruction allows for any encoding to be done - // for the "path" which includes the "realName" - var ssp = new StringBuilder(jarUri.getPath().length() + realName.length() + 2); - ssp.append(jarUri.getPath()); - ssp.append("!/"); - ssp.append(realName); - final URL resUrl = new URI(jarUri.getScheme(), ssp.toString(), null).toURL(); + final URL resUrl = getUrl(path, realName); // wrap it up into a "jar" protocol URL //horrible hack to deal with '?' characters in the URL //seems to be the only way, the URI constructor just does not let you handle them in a sane way @@ -136,8 +122,18 @@ public URL getResourceURL(String resource) { } catch (MalformedURLException | URISyntaxException e) { throw new RuntimeException(e); } - } finally { - readLock.unlock(); + } + + private static URL getUrl(Path jarPath, String realName) throws MalformedURLException, URISyntaxException { + final URI jarUri = jarPath.toUri(); + // first create a URI which includes both the jar file path and the relative resource name + // and then invoke a toURL on it. The URI reconstruction allows for any encoding to be done + // for the "path" which includes the "realName" + var ssp = new StringBuilder(jarUri.getPath().length() + realName.length() + 2); + ssp.append(jarUri.getPath()); + ssp.append("!/"); + ssp.append(realName); + return new URI(jarUri.getScheme(), ssp.toString(), null).toURL(); } } @@ -151,60 +147,11 @@ public ProtectionDomain getProtectionDomain() { return protectionDomain; } - private JarFile readLockAcquireAndGetJarReference() { - while (true) { - readLock.lock(); - final JarFile zipFileLocal = this.zipFile; - if (zipFileLocal != null) { - //Expected fast path: returns a reference to the open JarFile while owning the readLock - return zipFileLocal; - } else { - //This Lock implementation doesn't allow upgrading a readLock to a writeLock, so release it - //as we're going to need the WriteLock. - readLock.unlock(); - //trigger the JarFile being (re)opened. - ensureJarFileIsOpen(); - //Now since we no longer own any lock, we need to try again to obtain the readLock - //and check for the reference still being valid. - //This exposes us to a race with closing the just-opened JarFile; - //however this should be extremely rare, so we can trust we won't loop much; - //A counter doesn't seem necessary, as in fact we know that methods close() - //and resetInternalCaches() are invoked each at most once, which limits the amount - //of loops here in practice. - } - } - } - - private void ensureJarFileIsOpen() { - writeLock.lock(); - try { - if (this.zipFile == null) { - try { - this.zipFile = JarFiles.create(jarPath.toFile()); - } catch (IOException e) { - throw new RuntimeException("Failed to open " + jarPath, e); - } - } - } finally { - writeLock.unlock(); - } - } - @Override public void close() { - writeLock.lock(); - try { - final JarFile zipFileLocal = this.zipFile; - if (zipFileLocal != null) { - try { - this.zipFile = null; - zipFileLocal.close(); - } catch (Throwable e) { - //ignore - } - } - } finally { - writeLock.unlock(); + JarFileReference ref = jarFileReference.get(); + if (ref != null) { + ref.close(); } }