From 090d0e613fc82e30ab205fd099bac2a08ef31731 Mon Sep 17 00:00:00 2001 From: mariofusco Date: Tue, 23 Jul 2024 15:08:14 +0200 Subject: [PATCH] Revert "Replace read/write lock in JarResource to avoid virtual threads pinning" This reverts commit ff4b32bb0d8a1df0ccf1cc86a1e1ae9a840c5b0f. --- .../bootstrap/runner/JarFileReference.java | 172 ------------------ .../quarkus/bootstrap/runner/JarResource.java | 140 +++++++++----- .../bootstrap/runner/RunnerClassLoader.java | 93 ++++------ .../runner/VirtualThreadSupport.java | 52 ------ 4 files changed, 127 insertions(+), 330 deletions(-) delete mode 100644 independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/JarFileReference.java delete mode 100644 independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/VirtualThreadSupport.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 deleted file mode 100644 index d798f20830900..0000000000000 --- a/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/JarFileReference.java +++ /dev/null @@ -1,172 +0,0 @@ -package io.quarkus.bootstrap.runner; - -import static io.quarkus.bootstrap.runner.VirtualThreadSupport.isVirtualThread; - -import java.io.IOException; -import java.nio.file.Path; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.atomic.AtomicInteger; -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; - - // 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 JarFileReference is created as already acquired and that's why the referenceCounter starts from 2 - private final AtomicInteger referenceCounter = new AtomicInteger(2); - - private JarFileReference(JarFile jarFile) { - this.jarFile = jarFile; - } - - /** - * Increase the readers counter of the jarFile. - * - * @return true if the acquiring 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(JarResource jarResource) { - 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 { - jarResource.jarFileReference.set(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(JarResource jarResource) { - release(jarResource); - } - - @FunctionalInterface - interface JarFileConsumer { - T apply(JarFile jarFile, Path jarPath, String resource); - } - - static T withJarFile(JarResource jarResource, String resource, JarFileConsumer fileConsumer) { - - // Happy path: the jar reference already exists and it's ready to be used - final var localJarFileRefFuture = jarResource.jarFileReference.get(); - if (localJarFileRefFuture != null && localJarFileRefFuture.isDone()) { - JarFileReference jarFileReference = localJarFileRefFuture.join(); - if (jarFileReference.acquire()) { - return consumeSharedJarFile(jarFileReference, jarResource, resource, fileConsumer); - } - } - - // There's no valid jar reference, so load a new one - - // Platform threads can load the jarfile asynchronously and eventually blocking till not ready - // to avoid loading the same jarfile multiple times in parallel - if (!isVirtualThread()) { - // It's ok to eventually block on a join() here since we're sure this is used only by platform thread - return consumeSharedJarFile(asyncLoadAcquiredJarFile(jarResource).join(), jarResource, resource, fileConsumer); - } - - // Virtual threads needs to load the jarfile synchronously to avoid blocking. This means that eventually - // multiple threads could load the same jarfile in parallel and this duplication has to be reconciled - final var newJarFileRef = syncLoadAcquiredJarFile(jarResource); - if (jarResource.jarFileReference.compareAndSet(localJarFileRefFuture, newJarFileRef) || - jarResource.jarFileReference.compareAndSet(null, newJarFileRef)) { - // The new file reference has been successfully published and can be used by the current and other threads - // The join() cannot be blocking here since the CompletableFuture has been created already completed - return consumeSharedJarFile(newJarFileRef.join(), jarResource, resource, fileConsumer); - } - - // The newly created file reference hasn't been published, so it can be used exclusively by the current virtual thread - return consumeUnsharedJarFile(newJarFileRef, jarResource, resource, fileConsumer); - } - - private static T consumeSharedJarFile(JarFileReference jarFileReference, - JarResource jarResource, String resource, JarFileConsumer fileConsumer) { - try { - return fileConsumer.apply(jarFileReference.jarFile, jarResource.jarPath, resource); - } finally { - jarFileReference.release(jarResource); - } - } - - private static T consumeUnsharedJarFile(CompletableFuture jarFileReferenceFuture, - JarResource jarResource, String resource, JarFileConsumer fileConsumer) { - JarFileReference jarFileReference = jarFileReferenceFuture.join(); - try { - return fileConsumer.apply(jarFileReference.jarFile, jarResource.jarPath, resource); - } finally { - boolean closed = jarFileReference.release(jarResource); - assert !closed; - // Check one last time if the file reference can be published and reused by other threads, otherwise close it - if (!jarResource.jarFileReference.compareAndSet(null, jarFileReferenceFuture)) { - closed = jarFileReference.release(jarResource); - assert closed; - } - } - } - - private static CompletableFuture syncLoadAcquiredJarFile(JarResource jarResource) { - try { - return CompletableFuture.completedFuture(new JarFileReference(JarFiles.create(jarResource.jarPath.toFile()))); - } catch (IOException e) { - throw new RuntimeException("Failed to open " + jarResource.jarPath, e); - } - } - - private static CompletableFuture asyncLoadAcquiredJarFile(JarResource jarResource) { - CompletableFuture newJarFileRef = new CompletableFuture<>(); - do { - if (jarResource.jarFileReference.compareAndSet(null, newJarFileRef)) { - try { - newJarFileRef.complete(new JarFileReference(JarFiles.create(jarResource.jarPath.toFile()))); - return newJarFileRef; - } catch (IOException e) { - throw new RuntimeException(e); - } - } - newJarFileRef = jarResource.jarFileReference.get(); - } while (newJarFileRef == null || !newJarFileRef.join().acquire()); - return newJarFileRef; - } -} 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 84ae3b69246a2..8b4da096a891d 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,28 +11,44 @@ import java.security.ProtectionDomain; import java.security.cert.Certificate; import java.util.Objects; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; 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 */ public class JarResource implements ClassLoadingResource { - private volatile ProtectionDomain protectionDomain; private final ManifestInfo manifestInfo; + private final Path jarPath; + + private final Lock readLock; + private final Lock writeLock; + + private volatile ProtectionDomain protectionDomain; - final Path jarPath; - final AtomicReference> jarFileReference = new AtomicReference<>(); + //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; 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 @@ -53,48 +69,38 @@ public void init() { @Override public byte[] getResourceData(String resource) { - return JarFileReference.withJarFile(this, 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); + final ZipFile zipFile = readLockAcquireAndGetJarReference(); + try { + ZipEntry entry = zipFile.getEntry(resource); if (entry == null) { return null; } - try (InputStream is = jarFile.getInputStream(entry)) { + try (InputStream is = zipFile.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 " + res); + throw new RuntimeException("Failed to read all data for " + resource); } pos += read; rem -= read; } return data; } catch (IOException e) { - throw new RuntimeException("Failed to read zip entry " + res, e); + throw new RuntimeException("Failed to read zip entry " + resource, e); } + } finally { + readLock.unlock(); } } @Override public URL getResourceURL(String resource) { - return JarFileReference.withJarFile(this, 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); + final JarFile jarFile = readLockAcquireAndGetJarReference(); + try { + JarEntry entry = jarFile.getJarEntry(resource); if (entry == null) { return null; } @@ -104,7 +110,15 @@ public URL apply(JarFile jarFile, Path path, String res) { if (realName.endsWith("/")) { realName = realName.substring(0, realName.length() - 1); } - final URL resUrl = getUrl(path, realName); + 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(); // 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 @@ -122,18 +136,8 @@ public URL apply(JarFile jarFile, Path path, String res) { } catch (MalformedURLException | URISyntaxException e) { throw new RuntimeException(e); } - } - - 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(); + } finally { + readLock.unlock(); } } @@ -147,16 +151,60 @@ 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() { - 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.close(this); + writeLock.lock(); + try { + final JarFile zipFileLocal = this.zipFile; + if (zipFileLocal != null) { + try { + this.zipFile = null; + zipFileLocal.close(); + } catch (Throwable e) { + //ignore + } } + } finally { + writeLock.unlock(); } } diff --git a/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/RunnerClassLoader.java b/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/RunnerClassLoader.java index 2ff5b966de87a..7917d17b851f0 100644 --- a/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/RunnerClassLoader.java +++ b/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/RunnerClassLoader.java @@ -28,10 +28,6 @@ */ public final class RunnerClassLoader extends ClassLoader { - static { - registerAsParallelCapable(); - } - /** * A map of resources by dir name. Root dir/default package is represented by the empty string */ @@ -107,55 +103,18 @@ public Class loadClass(String name, boolean resolve) throws ClassNotFoundExce continue; } definePackage(packageName, resources); - return defineClass(name, data, resource); - } - } - return getParent().loadClass(name); - } - - private void definePackage(String pkgName, ClassLoadingResource[] resources) { - if ((pkgName != null) && getDefinedPackage(pkgName) == null) { - for (ClassLoadingResource classPathElement : resources) { - ManifestInfo mf = classPathElement.getManifestInfo(); - if (mf != null) { - try { - definePackage(pkgName, mf.getSpecTitle(), - mf.getSpecVersion(), - mf.getSpecVendor(), - mf.getImplTitle(), - mf.getImplVersion(), - mf.getImplVendor(), null); - } catch (IllegalArgumentException e) { - var loaded = getDefinedPackage(pkgName); - if (loaded == null) { - throw e; - } + try { + return defineClass(name, data, 0, data.length, resource.getProtectionDomain()); + } catch (LinkageError e) { + loaded = findLoadedClass(name); + if (loaded != null) { + return loaded; } - return; - } - } - try { - definePackage(pkgName, null, null, null, null, null, null, null); - } catch (IllegalArgumentException e) { - var loaded = getDefinedPackage(pkgName); - if (loaded == null) { throw e; } } } - } - - private Class defineClass(String name, byte[] data, ClassLoadingResource resource) { - Class loaded; - try { - return defineClass(name, data, 0, data.length, resource.getProtectionDomain()); - } catch (LinkageError e) { - loaded = findLoadedClass(name); - if (loaded != null) { - return loaded; - } - throw e; - } + return getParent().loadClass(name); } private void accessingResource(final ClassLoadingResource resource) { @@ -172,33 +131,25 @@ private void accessingResource(final ClassLoadingResource resource) { //it's already on the head of the cache: nothing to be done. return; } - for (int i = 1; i < currentlyBufferedResources.length; i++) { final ClassLoadingResource currentI = currentlyBufferedResources[i]; if (currentI == resource || currentI == null) { //it was already cached, or we found an empty slot: bubble it up by one position to give it a boost - bubbleUpCachedResource(resource, i); + final ClassLoadingResource previous = currentlyBufferedResources[i - 1]; + currentlyBufferedResources[i - 1] = resource; + currentlyBufferedResources[i] = previous; return; } } - // else, we drop one element from the cache, // and inserting the latest resource on the tail: toEvict = currentlyBufferedResources[currentlyBufferedResources.length - 1]; - bubbleUpCachedResource(resource, currentlyBufferedResources.length - 1); + currentlyBufferedResources[currentlyBufferedResources.length - 1] = resource; } - // Finally, release the cache for the dropped element: toEvict.resetInternalCaches(); } - private void bubbleUpCachedResource(ClassLoadingResource resource, int i) { - for (int j = i; j > 0; j--) { - currentlyBufferedResources[j] = currentlyBufferedResources[j - 1]; - } - currentlyBufferedResources[0] = resource; - } - @Override protected URL findResource(String name) { name = sanitizeName(name); @@ -270,6 +221,28 @@ protected Enumeration findResources(String name) { return Collections.enumeration(urls); } + private void definePackage(String pkgName, ClassLoadingResource[] resources) { + if ((pkgName != null) && getPackage(pkgName) == null) { + synchronized (getClassLoadingLock(pkgName)) { + if (getPackage(pkgName) == null) { + for (ClassLoadingResource classPathElement : resources) { + ManifestInfo mf = classPathElement.getManifestInfo(); + if (mf != null) { + definePackage(pkgName, mf.getSpecTitle(), + mf.getSpecVersion(), + mf.getSpecVendor(), + mf.getImplTitle(), + mf.getImplVersion(), + mf.getImplVendor(), null); + return; + } + } + definePackage(pkgName, null, null, null, null, null, null, null); + } + } + } + } + private String getPackageNameFromClassName(String className) { final int index = className.lastIndexOf('.'); if (index == -1) { diff --git a/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/VirtualThreadSupport.java b/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/VirtualThreadSupport.java deleted file mode 100644 index 5d6f03a51a3ab..0000000000000 --- a/independent-projects/bootstrap/runner/src/main/java/io/quarkus/bootstrap/runner/VirtualThreadSupport.java +++ /dev/null @@ -1,52 +0,0 @@ -package io.quarkus.bootstrap.runner; - -import java.lang.invoke.MethodHandle; -import java.lang.invoke.MethodHandles; -import java.lang.invoke.MethodType; - -public class VirtualThreadSupport { - - private static final int MAJOR_JAVA_VERSION = majorVersionFromJavaSpecificationVersion(); - - private static final MethodHandle virtualMh = MAJOR_JAVA_VERSION >= 21 ? findVirtualMH() : null; - - private static MethodHandle findVirtualMH() { - try { - return MethodHandles.publicLookup().findVirtual(Thread.class, "isVirtual", - MethodType.methodType(boolean.class)); - } catch (Exception e) { - return null; - } - } - - static boolean isVirtualThread() { - if (virtualMh == null) { - return false; - } - try { - return (boolean) virtualMh.invokeExact(Thread.currentThread()); - } catch (Throwable t) { - return false; - } - } - - static int majorVersionFromJavaSpecificationVersion() { - return majorVersion(System.getProperty("java.specification.version", "17")); - } - - static int majorVersion(String javaSpecVersion) { - String[] components = javaSpecVersion.split("\\."); - int[] version = new int[components.length]; - - for (int i = 0; i < components.length; ++i) { - version[i] = Integer.parseInt(components[i]); - } - - if (version[0] == 1) { - assert version[1] >= 6; - return version[1]; - } else { - return version[0]; - } - } -}