Skip to content

Created a better thread safety test and fixed a couple thread safety issues #395

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ivakegg wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Created a better thread safety test and fixed a couple thread safety issues #395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
db94749
re #836: VFSClassLoader multi-thread resource loading
ivakegg May 3, 2023
ace31a8
re #836: Threaded VFS safety
ivakegg May 10, 2023
f7899db
re #836: Removed redundant modifiers
ivakegg May 10, 2023
8e5be25
re #836: Reverted to explicitly include new test cases instead of
ivakegg May 10, 2023
bb4f8af
re #836: removed unused imports
ivakegg May 10, 2023
e152b17
re #836: removed thread safety test for resource provider
ivakegg May 10, 2023
a125800
re #836: inadvertant change reverted
ivakegg May 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 56 additions & 17 deletions commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/tar/TarFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,54 @@ public class TarFileSystem extends AbstractFileSystem {

private final File file;

private TarArchiveInputStream tarFile;
private TarFileThreadLocal tarFile = new TarFileThreadLocal();

private class TarFileCreationException extends RuntimeException {
TarFileCreationException(Throwable cause) {
super(cause);
}
}

private class TarFileThreadLocal {

private ThreadLocal<Boolean> isPresent = new ThreadLocal<Boolean>() {
@Override
protected Boolean initialValue() {
return Boolean.FALSE;
}
};
private ThreadLocal<TarArchiveInputStream> tarFile = new ThreadLocal<TarArchiveInputStream>() {
@Override
public TarArchiveInputStream initialValue() {
if (isPresent.get()) {
throw new IllegalStateException("Creating an initial value but we already have one");
}
try {
isPresent.set(Boolean.TRUE);
return createTarFile(TarFileSystem.this.file);
} catch (FileSystemException fse) {
throw new TarFileCreationException(fse);
}
}
};

public TarArchiveInputStream getFile() throws FileSystemException {
try {
return tarFile.get();
} catch (TarFileCreationException e) {
throw new FileSystemException(e);
}
}

public void closeFile() throws IOException {
if (isPresent.get()) {
TarArchiveInputStream file = tarFile.get();
file.close();
tarFile.remove();
isPresent.set(Boolean.FALSE);
}
}
}

/**
* Cache doesn't need to be synchronized since it is read-only.
Expand Down Expand Up @@ -117,10 +164,7 @@ protected TarFileObject createTarFileObject(final AbstractFileName name, final T
protected void doCloseCommunicationLink() {
// Release the tar file
try {
if (tarFile != null) {
tarFile.close();
tarFile = null;
}
tarFile.closeFile();
} catch (final IOException e) {
// getLogger().warn("vfs.provider.tar/close-tar-file.error :" + file, e);
VfsLog.warn(getLogger(), LOG, "vfs.provider.tar/close-tar-file.error :" + file, e);
Expand All @@ -147,6 +191,7 @@ public InputStream getInputStream(final TarArchiveEntry entry) throws FileSystem
resetTarFile();
try {
ArchiveEntry next;
TarArchiveInputStream tarFile = getTarFile();
while ((next = tarFile.getNextEntry()) != null) {
if (next.equals(entry)) {
return tarFile;
Expand All @@ -159,10 +204,7 @@ public InputStream getInputStream(final TarArchiveEntry entry) throws FileSystem
}

protected TarArchiveInputStream getTarFile() throws FileSystemException {
if (tarFile == null && this.file.exists()) {
recreateTarFile();
}
return tarFile;
return tarFile.getFile();
}

@Override
Expand Down Expand Up @@ -225,15 +267,12 @@ protected void putFileToCache(final FileObject file) {
*/

private void recreateTarFile() throws FileSystemException {
if (this.tarFile != null) {
try {
this.tarFile.close();
} catch (final IOException e) {
throw new FileSystemException("vfs.provider.tar/close-tar-file.error", file, e);
}
tarFile = null;
try {
tarFile.closeFile();
} catch (final IOException e) {
throw new FileSystemException("vfs.provider.tar/close-tar-file.error", file, e);
}
this.tarFile = createTarFile(this.file);
tarFile.getFile();
}

/**
Expand Down
68 changes: 51 additions & 17 deletions commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/zip/ZipFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,54 @@ public class ZipFileSystem extends AbstractFileSystem {

private final File file;
private final Charset charset;
private ZipFile zipFile;
private ZipFileThreadLocal zipFile = new ZipFileThreadLocal();

private class ZipFileCreationException extends RuntimeException {
ZipFileCreationException(Throwable cause) {
super(cause);
}
}

private class ZipFileThreadLocal {

private ThreadLocal<Boolean> isPresent = new ThreadLocal<Boolean>() {
@Override
protected Boolean initialValue() {
return Boolean.FALSE;
}
};
private ThreadLocal<ZipFile> zipFile = new ThreadLocal<ZipFile>() {
@Override
public ZipFile initialValue() {
if (isPresent.get()) {
throw new IllegalStateException("Creating an initial value but we already have one");
}
try {
isPresent.set(Boolean.TRUE);
return createZipFile(ZipFileSystem.this.file);
} catch (FileSystemException fse) {
throw new ZipFileCreationException(fse);
}
}
};

public ZipFile getFile() throws FileSystemException {
try {
return zipFile.get();
} catch (ZipFileCreationException e) {
throw new FileSystemException(e);
}
}

public void closeFile() throws IOException {
if (isPresent.get()) {
ZipFile file = zipFile.get();
file.close();
zipFile.remove();
isPresent.set(Boolean.FALSE);
}
}
}

/**
* Cache doesn't need to be synchronized since it is read-only.
Expand All @@ -71,12 +118,6 @@ public ZipFileSystem(final AbstractFileName rootFileName, final FileObject paren
// Make a local copy of the file
file = parentLayer.getFileSystem().replicateFile(parentLayer, Selectors.SELECT_SELF);
this.charset = ZipFileSystemConfigBuilder.getInstance().getCharset(fileSystemOptions);

// Open the Zip file
if (!file.exists()) {
// Don't need to do anything
zipFile = null;
}
}

/**
Expand Down Expand Up @@ -111,12 +152,9 @@ protected ZipFileObject createZipFileObject(final AbstractFileName name, final Z

@Override
protected void doCloseCommunicationLink() {
// Release the zip file
// Release the zip files
try {
if (zipFile != null) {
zipFile.close();
zipFile = null;
}
zipFile.closeFile();
} catch (final IOException e) {
// getLogger().warn("vfs.provider.zip/close-zip-file.error :" + file, e);
VfsLog.warn(getLogger(), LOG, "vfs.provider.zip/close-zip-file.error :" + file, e);
Expand All @@ -136,11 +174,7 @@ protected FileObject getFileFromCache(final FileName name) {
}

protected ZipFile getZipFile() throws FileSystemException {
if (zipFile == null && this.file.exists()) {
this.zipFile = createZipFile(this.file);
}

return zipFile;
return zipFile.getFile();
}

@Override
Expand Down
99 changes: 0 additions & 99 deletions commons-vfs2/src/test/java/org/apache/commons/vfs2/impl/VfsClassLoaderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,11 @@
import static org.apache.commons.vfs2.VfsTestUtils.getTestDirectoryFile;

import java.io.File;
import java.io.PrintWriter;
import java.net.URL;
import java.net.URLConnection;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Enumeration;
import java.util.LinkedList;
import java.util.List;
import java.util.Queue;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.RejectedExecutionHandler;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;

import org.apache.commons.io.output.StringBuilderWriter;
import org.apache.commons.vfs2.AbstractProviderTestCase;
import org.apache.commons.vfs2.Capability;
import org.apache.commons.vfs2.FileObject;
Expand Down Expand Up @@ -215,93 +203,6 @@ public void testSealing() throws Exception {
verifyPackage(pack, true);
}

@Test
public void testThreadSafety() throws Exception {
final int THREADS = 40;
final BlockingQueue<Runnable> workQueue = new ArrayBlockingQueue<>(THREADS * 2);
final List<Throwable> exceptions = new ArrayList<>();
final Thread.UncaughtExceptionHandler handler = new Thread.UncaughtExceptionHandler() {
@Override
public void uncaughtException(Thread t, Throwable e) {
synchronized (exceptions) {
exceptions.add(e);
}
}
};
final ThreadFactory factory = new ThreadFactory() {
@Override
public Thread newThread(Runnable r) {
Thread thread = new Thread(r, "VfsClassLoaderTests.testThreadSafety");
thread.setUncaughtExceptionHandler(handler);
return thread;
}
};
final Queue<Runnable> rejections = new LinkedList<>();
final RejectedExecutionHandler rejectionHandler = new RejectedExecutionHandler() {
@Override
public void rejectedExecution(Runnable r, ThreadPoolExecutor executor) {
synchronized (rejections) {
rejections.add(r);
}
}
};
final ThreadPoolExecutor executor = new ThreadPoolExecutor(THREADS, THREADS, 0, TimeUnit.SECONDS, workQueue, factory, rejectionHandler);
executor.prestartAllCoreThreads();
for (int i = 0; i < THREADS; i++) {
final VFSClassLoader loader = createClassLoader();
workQueue.put(new VfsClassLoaderTests.LoadClass(loader));
}
while (!workQueue.isEmpty()) {
Thread.sleep(10);
}
while (!rejections.isEmpty() && executor.getActiveCount() > 0) {
final List<Runnable> rejected = new ArrayList<>();
synchronized(rejections) {
rejected.addAll(rejections);
rejections.clear();
}
workQueue.addAll(rejected);
}
executor.shutdown();
executor.awaitTermination(30, TimeUnit.SECONDS);
assertEquals(THREADS, executor.getCompletedTaskCount());
if (!exceptions.isEmpty()) {
StringBuilder exceptionMsg = new StringBuilder();
StringBuilderWriter writer = new StringBuilderWriter(exceptionMsg);
PrintWriter pWriter = new PrintWriter(writer);
for (Throwable t : exceptions) {
pWriter.write(t.getMessage());
pWriter.write('\n');
t.printStackTrace(pWriter);
pWriter.write('\n');
}
pWriter.flush();
assertTrue(exceptions.size() + " threads failed: " + exceptionMsg, exceptions.isEmpty());
}
}

private class LoadClass implements Runnable {
private final VFSClassLoader loader;
public LoadClass(VFSClassLoader loader) {
this.loader = loader;
}

@Override
public void run() {
try {
final Class<?> testClass = loader.findClass("code.ClassToLoad");
final Package pack = testClass.getPackage();
assertEquals("code", pack.getName());
verifyPackage(pack, false);

final Object testObject = testClass.newInstance();
assertEquals("**PRIVATE**", testObject.toString());
} catch (ClassNotFoundException | InstantiationException | IllegalAccessException e) {
throw new RuntimeException(e);
}
}
}

/**
* Verify the package loaded with class loader.
*/
Expand Down
Loading