Merge pull request #42083 from mariofusco/revert-cl

Revert "Replace read/write lock in JarResource to avoid virtual threads pinning"

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<CompletableFuture<JarFileReference>> 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<byte[]> {
-        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<URL> {
-        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();
         }
     }