From 29109e24b932a450c33cf0813d939d780f8507cb Mon Sep 17 00:00:00 2001 From: Stefan Winkler Date: Mon, 7 Aug 2023 08:18:33 +0000 Subject: [PATCH] Improved exception and shutdown handling - Added join for the CacheWriter thread, so it is not terminated hard - Improved exception and interrupt handling, deleting files if they have a risk of being incomplete --- .../caching/BundleCachingService.java | 6 ++- .../weaving/internal/caching/CacheWriter.java | 54 +++++++++++++++---- .../caching/ClassnameLockManager.java | 13 ++--- 3 files changed, 50 insertions(+), 23 deletions(-) diff --git a/bundles/org.eclipse.equinox.weaving.caching/src/org/eclipse/equinox/weaving/internal/caching/BundleCachingService.java b/bundles/org.eclipse.equinox.weaving.caching/src/org/eclipse/equinox/weaving/internal/caching/BundleCachingService.java index dbdec4a97ac..0ffa292d9a4 100644 --- a/bundles/org.eclipse.equinox.weaving.caching/src/org/eclipse/equinox/weaving/internal/caching/BundleCachingService.java +++ b/bundles/org.eclipse.equinox.weaving.caching/src/org/eclipse/equinox/weaving/internal/caching/BundleCachingService.java @@ -140,9 +140,11 @@ public CacheEntry findStoredClass(final String namespace, final URL sourceFileUR // so, take it from there storedClass = itemsInQueue.get(new CacheItemKey(directoryString, name)); if (storedClass == null) { - // else, read it from disk + // else, read it from disk (if it exists) final File cachedBytecodeFile = new File(cacheDirectory, name); - storedClass = lockManager.executeRead(name, () -> read(name, cachedBytecodeFile)); + if (cachedBytecodeFile.exists()) { + storedClass = lockManager.executeRead(name, () -> read(name, cachedBytecodeFile)); + } } isCached = storedClass != null; } diff --git a/bundles/org.eclipse.equinox.weaving.caching/src/org/eclipse/equinox/weaving/internal/caching/CacheWriter.java b/bundles/org.eclipse.equinox.weaving.caching/src/org/eclipse/equinox/weaving/internal/caching/CacheWriter.java index 08af5994be5..90abe2a189b 100644 --- a/bundles/org.eclipse.equinox.weaving.caching/src/org/eclipse/equinox/weaving/internal/caching/CacheWriter.java +++ b/bundles/org.eclipse.equinox.weaving.caching/src/org/eclipse/equinox/weaving/internal/caching/CacheWriter.java @@ -19,9 +19,9 @@ import java.io.BufferedOutputStream; import java.io.DataOutputStream; import java.io.File; -import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; +import java.nio.file.Files; import java.util.Map; import java.util.Map.Entry; import java.util.concurrent.BlockingQueue; @@ -33,6 +33,12 @@ * @author Martin Lippert */ public class CacheWriter { + /** + * A timeout value in milliseconds to wait for the writer thread to finish after + * signalling the interrupt. + */ + private static final long JOIN_TIMEOUT = 5000L; + /** * A map for items that are currently contained in the {@link #cacheWriterQueue} */ @@ -62,13 +68,10 @@ public CacheWriter(final BlockingQueue cacheQueue, final Map generatedClasses = item.getGeneratedClasses(); if (generatedClasses != null) { @@ -115,22 +125,44 @@ protected void store(final CacheItem item) throws IOException { () -> storeSingleClass(item.getName(), item.getCachedBytes(), item.getDirectory())); } - private void storeSingleClass(final String className, final byte[] classBytes, final String cacheDirectory) - throws FileNotFoundException, IOException { + private void storeSingleClass(final String className, final byte[] classBytes, final String cacheDirectory) { final File directory = new File(cacheDirectory); if (!directory.exists()) { directory.mkdirs(); } - try (FileOutputStream fosCache = new FileOutputStream(new File(directory, className)); + final File outputFile = new File(directory, className); + boolean success = true; + try (FileOutputStream fosCache = new FileOutputStream(outputFile); DataOutputStream outCache = new DataOutputStream(new BufferedOutputStream(fosCache))) { outCache.write(classBytes); outCache.flush(); fosCache.getFD().sync(); + } catch (IOException e) { + Log.error("Failed to store class " + className + " in cache", e); + success = false; + } + + // if there was an error during writing the file, or if an interrupt happened, + // we have a risk of having written an incomplete file. To be sure, we check + // the length of the file on disk. If it does not match, we delete the file + // again. + if ((!success || Thread.currentThread().isInterrupted()) && outputFile.exists() + && outputFile.length() != classBytes.length) { + Log.debug("File " + outputFile.getAbsolutePath() + " was not completely written to disk. Removing it."); + try { + Files.delete(outputFile.toPath()); + } catch (IOException e) { + Log.error("File " + outputFile.getAbsolutePath() + " is corrupted but could not be deleted.", e); + // last resort: try to delete the file when the VM terminates + outputFile.deleteOnExit(); + } } // after writing the file, remove the item from the itemsInQueue lookup map as - // well + // well - we do this even the writing was unsuccessful to keep the queue and the + // itemsInQueue in sync. It will do no further harm, just the file is not cached and will be woven and + // cached again next time. itemsInQueue.remove(new CacheItemKey(cacheDirectory, className)); } } diff --git a/bundles/org.eclipse.equinox.weaving.caching/src/org/eclipse/equinox/weaving/internal/caching/ClassnameLockManager.java b/bundles/org.eclipse.equinox.weaving.caching/src/org/eclipse/equinox/weaving/internal/caching/ClassnameLockManager.java index 432b8118323..8cb22b3b1a0 100644 --- a/bundles/org.eclipse.equinox.weaving.caching/src/org/eclipse/equinox/weaving/internal/caching/ClassnameLockManager.java +++ b/bundles/org.eclipse.equinox.weaving.caching/src/org/eclipse/equinox/weaving/internal/caching/ClassnameLockManager.java @@ -14,7 +14,6 @@ package org.eclipse.equinox.weaving.internal.caching; -import java.io.IOException; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.locks.Lock; @@ -76,11 +75,8 @@ public interface ReadOperation { public interface WriteOperation { /** * The logic to write a class file to the cache. - * - * @throws IOException in case of a file system error in the logic, this can be - * propagated to the caller. */ - void write() throws IOException; + void write(); } /** @@ -110,7 +106,7 @@ private Lock acquireLock(final String classname, final boolean writeLock) { // existingEntry == null means there is no lock for the given classname, so we // create one. final LockWithCount resultingEntry = existingEntry != null ? existingEntry : new LockWithCount(); - // ge the lock that we need (read or write, depending on the parameter). + // get the lock that we need (read or write, depending on the parameter). lockWrapper[0] = writeLock ? resultingEntry.lock.writeLock() : resultingEntry.lock.readLock(); // increment the lock counter resultingEntry.count.incrementAndGet(); @@ -150,11 +146,8 @@ public byte[] executeRead(final String classname, final ReadOperation readOperat * * @param classname the classname defining the scope of the lock to acquire * @param writeOperation the operation to execute - * - * @throws IOException any exception thrown by the writeOperation will be - * propagated to the caller */ - public void executeWrite(final String classname, final WriteOperation writeOperation) throws IOException { + public void executeWrite(final String classname, final WriteOperation writeOperation) { final Lock lock = acquireLock(classname, true); try { writeOperation.write();