garydgregory commented on code in PR #395:
URL: https://github.com/apache/commons-vfs/pull/395#discussion_r1186836956
##########
commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/tar/TarFileSystem.java:
##########
@@ -16,16 +16,6 @@
*/
package org.apache.commons.vfs2.provider.tar;
-import java.io.File;
Review Comment:
Don't mess with imports, please.
##########
commons-vfs2/src/test/java/org/apache/commons/vfs2/impl/VfsClassLoaderTests.java:
##########
@@ -217,7 +249,17 @@ public void testSealing() throws Exception {
@Test
public void testThreadSafety() throws Exception {
- final int THREADS = 40;
+ final FileSystemManager manager = getManager();
+
+ // The http4 and sftp mechanisms do not work with this thread safety
test
+ List<String> schemes = Arrays.asList(manager.getSchemes());
+ if (schemes.contains("http4") || schemes.contains("sftp")) {
Review Comment:
-1 This is not how to conditionally enable or disable tests in JUnit, and
gives the _false_ impression that a tests _passes_ when it is in fact _not even
run_! This kind of pattern can be handled either in each subclass in a disabled
test method with a comment or using a JUnit assume statement.
##########
commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/tar/TarFileSystem.java:
##########
@@ -54,7 +54,53 @@ public class TarFileSystem extends AbstractFileSystem {
private final File file;
- private TarArchiveInputStream tarFile;
+ private TarFileThreadLocal tarFile = new TarFileThreadLocal();
+
+ 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 RuntimeException(fse);
Review Comment:
See my other comment about RuntimeException in another file below.
##########
commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/zip/ZipFileSystem.java:
##########
@@ -49,7 +49,53 @@ public class ZipFileSystem extends AbstractFileSystem {
private final File file;
private final Charset charset;
- private ZipFile zipFile;
+ private ZipFileThreadLocal zipFile = new ZipFileThreadLocal();
+
+ 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 RuntimeException(fse);
Review Comment:
Throwing RuntimeException is an anti-pattern. Think about the problem you
want to report: Is this an illegal argument, an illegal state, or something
else like an actual IO exception that needs to be unchecked?
##########
commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/ftp/FtpProviderTestCase.java:
##########
@@ -113,6 +114,14 @@ static void setUpClass(final String rootDirectory, final
FileSystemFactory fileS
if (commandFactory != null) {
serverFactory.setCommandFactory(commandFactory);
}
+ ConnectionConfigFactory configFactory = new ConnectionConfigFactory();
Review Comment:
Follow the code convention in the file you are editing: In this case, use
`final`.
##########
commons-vfs2/src/test/java/org/apache/commons/vfs2/impl/VfsClassLoaderTests.java:
##########
@@ -263,14 +309,14 @@ public void rejectedExecution(Runnable r,
ThreadPoolExecutor executor) {
workQueue.addAll(rejected);
}
executor.shutdown();
- executor.awaitTermination(30, TimeUnit.SECONDS);
+ executor.awaitTermination(5, TimeUnit.MINUTES);
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(String.valueOf(t.getMessage()));
Review Comment:
Why?
##########
commons-vfs2/src/test/java/org/apache/commons/vfs2/impl/VfsClassLoaderTests.java:
##########
@@ -16,39 +16,71 @@
*/
package org.apache.commons.vfs2.impl;
-import static org.apache.commons.vfs2.VfsTestUtils.getTestDirectoryFile;
+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;
+import org.apache.commons.vfs2.FileSystemException;
+import org.apache.commons.vfs2.FileSystemManager;
+import org.apache.commons.vfs2.FileType;
+import org.apache.commons.vfs2.operations.FileOperationProvider;
+import org.junit.Test;
import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
import java.io.PrintWriter;
import java.net.URL;
import java.net.URLConnection;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collections;
import java.util.Enumeration;
import java.util.LinkedList;
import java.util.List;
+import java.util.Map;
import java.util.Queue;
+import java.util.Set;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.BrokenBarrierException;
+import java.util.concurrent.CyclicBarrier;
import java.util.concurrent.RejectedExecutionHandler;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
-import org.apache.commons.io.output.StringBuilderWriter;
Review Comment:
Don't reorder, the fewer changes in a PR, the easier it is to review.
##########
commons-vfs2/src/test/java/org/apache/commons/vfs2/impl/VfsClassLoaderTests.java:
##########
@@ -263,14 +309,14 @@ public void rejectedExecution(Runnable r,
ThreadPoolExecutor executor) {
workQueue.addAll(rejected);
}
executor.shutdown();
- executor.awaitTermination(30, TimeUnit.SECONDS);
+ executor.awaitTermination(5, TimeUnit.MINUTES);
Review Comment:
I don't think we want to tests to possibly take "forever" when they might
fail anyway.
##########
commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/zip/ZipFileSystem.java:
##########
@@ -39,6 +29,16 @@
import org.apache.commons.vfs2.provider.AbstractFileSystem;
import org.apache.commons.vfs2.provider.UriParser;
+import java.io.File;
Review Comment:
-1 Don't relocate import statements.
##########
commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/zip/ZipFileSystem.java:
##########
@@ -49,7 +49,53 @@ public class ZipFileSystem extends AbstractFileSystem {
private final File file;
private final Charset charset;
- private ZipFile zipFile;
+ private ZipFileThreadLocal zipFile = new ZipFileThreadLocal();
+
+ 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 RuntimeException(fse);
+ }
+ }
+ };
+
+ public ZipFile getFile() throws FileSystemException {
+ try {
+ return zipFile.get();
+ } catch (RuntimeException e) {
+ if (e.getCause() instanceof FileSystemException) {
+ throw new FileSystemException(e.getCause());
+ }
+ else {
+ throw new RuntimeException(e);
Review Comment:
See above.
##########
commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/tar/TarFileSystem.java:
##########
@@ -54,7 +54,53 @@ public class TarFileSystem extends AbstractFileSystem {
private final File file;
- private TarArchiveInputStream tarFile;
+ private TarFileThreadLocal tarFile = new TarFileThreadLocal();
+
+ 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 RuntimeException(fse);
+ }
+ }
+ };
+
+ public TarArchiveInputStream getFile() throws FileSystemException {
+ try {
+ return tarFile.get();
+ } catch (RuntimeException e) {
+ if (e.getCause() instanceof FileSystemException) {
+ throw new FileSystemException(e.getCause());
+ }
+ else {
+ throw new RuntimeException(e);
Review Comment:
See my other comment about RuntimeException in another file below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
