[IO-718] FileUtils.checksumCRC32 and FileUtils.checksum are not thread

safe.

Author: Gary Gregory

src/changes/changes.xml

@@ -117,6 +117,9 @@ The <action> type attribute can be add,update,fix,remove.
       <action dev="ggregory" type="fix" due-to="Felix Rilling">
         Fix Typos in JavaDoc, Comments and Tests #201.
       </action>
+      <action issue="IO-718" dev="ggregory" type="fix" due-to="Robert Cooper, Gary Gregory">
+        FileUtils.checksumCRC32 and FileUtils.checksum are not thread safe.
+      </action>      
       <!-- ADD -->
       <action dev="ggregory" type="add" due-to="Gary Gregory">
         Add FileSystemProviders class.

src/main/java/org/apache/commons/io/CopyUtils.java

@@ -192,7 +192,7 @@ public static int copy(
             final Reader input,
             final Writer output)
                 throws IOException {
-        final char[] buffer = new char[IOUtils.DEFAULT_BUFFER_SIZE];
+        final char[] buffer = IOUtils.getCharArray();
         int count = 0;
         int n = 0;
         while (EOF != (n = input.read(buffer))) {

src/main/java/org/apache/commons/io/IOUtils.java

@@ -176,23 +176,15 @@ public class IOUtils {
     public static final String LINE_SEPARATOR_WINDOWS = StandardLineSeparator.CRLF.getString();
 
     /**
-     * The default buffer to use for the skip() methods.
+     * Internal byte array buffer.
      */
-    private static final byte[] SKIP_BYTE_BUFFER = byteArray();
-
-    // Allocated in the relevant skip method if necessary.
-    /*
-     * These buffers are static and are shared between threads.
-     * This is possible because the buffers are write-only - the contents are never read.
-     *
-     * N.B. there is no need to synchronize when creating these because:
-     * - we don't care if the buffer is created multiple times (the data is ignored)
-     * - we always use the same size buffer, so if it it is recreated it will still be OK
-     * (if the buffer size were variable, we would need to synch. to ensure some other thread
-     * did not create a smaller one)
+    private static final ThreadLocal<byte[]> SKIP_BYTE_BUFFER = ThreadLocal.withInitial(() -> byteArray());
+    
+    /**
+     * Internal byte array buffer.
      */
-    private static char[] SKIP_CHAR_BUFFER;
-
+    private static final ThreadLocal<char[]> SKIP_CHAR_BUFFER = ThreadLocal.withInitial(() -> charArray());
+    
     /**
      * Returns the given InputStream if it is already a {@link BufferedInputStream}, otherwise creates a
      * BufferedInputStream from the given InputStream.
@@ -344,6 +336,29 @@ public static byte[] byteArray(final int size) {
         return new byte[size];
     }
 
+    /**
+     * Returns a new char array of size {@link #DEFAULT_BUFFER_SIZE}.
+     *
+     * @return a new char array of size {@link #DEFAULT_BUFFER_SIZE}.
+     * @since 2.9.0
+     */
+    private static char[] charArray() {
+        return charArray(DEFAULT_BUFFER_SIZE);
+    }
+
+    /**
+     * Returns a new char array of the given size.
+     *
+     * TODO Consider guarding or warning against large allocations...
+     *
+     * @param size array size.
+     * @return a new char array of the given size.
+     * @since 2.9.0
+     */
+    private static char[] charArray(final int size) {
+        return new char[size];
+    }
+
     /**
      * Closes the given {@link Closeable} as a null-safe operation.
      *
@@ -755,7 +770,7 @@ public static void closeQuietly(final Writer writer) {
      */
     public static long consume(final InputStream input)
             throws IOException {
-        return copyLarge(input, NullOutputStream.NULL_OUTPUT_STREAM, SKIP_BYTE_BUFFER);
+        return copyLarge(input, NullOutputStream.NULL_OUTPUT_STREAM, getByteArray());
     }
 
     /**
@@ -783,7 +798,9 @@ public static boolean contentEquals(final InputStream input1, final InputStream
             return false;
         }
 
-        final byte[] array1 = byteArray();
+        // reuse one
+        final byte[] array1 = getByteArray();
+        // allocate another
         final byte[] array2 = byteArray();
         int pos1;
         int pos2;
@@ -839,8 +856,10 @@ public static boolean contentEquals(final Reader input1, final Reader input2) th
             return false;
         }
 
-        final char[] array1 = new char[DEFAULT_BUFFER_SIZE];
-        final char[] array2 = new char[DEFAULT_BUFFER_SIZE];
+        // reuse one
+        final char[] array1 = getCharArray();
+        // but allocate another
+        final char[] array2 = charArray();
         int pos1;
         int pos2;
         int count1;
@@ -1318,7 +1337,7 @@ public static long copyLarge(final InputStream inputStream, final OutputStream o
      */
     public static long copyLarge(final InputStream input, final OutputStream output, final long inputOffset,
                                  final long length) throws IOException {
-        return copyLarge(input, output, inputOffset, length, byteArray());
+        return copyLarge(input, output, inputOffset, length, getByteArray());
     }
 
     /**
@@ -1387,7 +1406,7 @@ public static long copyLarge(final InputStream input, final OutputStream output,
      * @since 1.3
      */
     public static long copyLarge(final Reader reader, final Writer writer) throws IOException {
-        return copyLarge(reader, writer, new char[DEFAULT_BUFFER_SIZE]);
+        return copyLarge(reader, writer, getCharArray());
     }
 
     /**
@@ -1436,7 +1455,7 @@ public static long copyLarge(final Reader reader, final Writer writer, final cha
      */
     public static long copyLarge(final Reader reader, final Writer writer, final long inputOffset, final long length)
             throws IOException {
-        return copyLarge(reader, writer, inputOffset, length, new char[DEFAULT_BUFFER_SIZE]);
+        return copyLarge(reader, writer, inputOffset, length, getCharArray());
     }
 
     /**
@@ -1484,6 +1503,24 @@ public static long copyLarge(final Reader reader, final Writer writer, final lon
         return totalRead;
     }
 
+    /**
+     * Gets the thread local byte array.
+     *
+     * @return the thread local byte array.
+     */
+    static byte[] getByteArray() {
+        return SKIP_BYTE_BUFFER.get();
+    }
+
+    /**
+     * Gets the thread local char array.
+     *
+     * @return the thread local char array.
+     */
+    static char[] getCharArray() {
+        return SKIP_CHAR_BUFFER.get();
+    }
+
     /**
      * Returns the length of the given array in a null-safe manner.
      *
@@ -2113,7 +2150,8 @@ public static long skip(final InputStream input, final long toSkip) throws IOExc
         long remain = toSkip;
         while (remain > 0) {
             // See https://issues.apache.org/jira/browse/IO-203 for why we use read() rather than delegating to skip()
-            final long n = input.read(SKIP_BYTE_BUFFER, 0, (int) Math.min(remain, SKIP_BYTE_BUFFER.length));
+            final byte[] byteArray = getByteArray();
+            final long n = input.read(byteArray, 0, (int) Math.min(remain, byteArray.length));
             if (n < 0) { // EOF
                 break;
             }
@@ -2138,11 +2176,11 @@ public static long skip(final ReadableByteChannel input, final long toSkip) thro
         if (toSkip < 0) {
             throw new IllegalArgumentException("Skip count must be non-negative, actual: " + toSkip);
         }
-        final ByteBuffer skipByteBuffer = ByteBuffer.allocate((int) Math.min(toSkip, SKIP_BYTE_BUFFER.length));
+        final ByteBuffer skipByteBuffer = ByteBuffer.allocate((int) Math.min(toSkip, DEFAULT_BUFFER_SIZE));
         long remain = toSkip;
         while (remain > 0) {
             skipByteBuffer.position(0);
-            skipByteBuffer.limit((int) Math.min(remain, SKIP_BYTE_BUFFER.length));
+            skipByteBuffer.limit((int) Math.min(remain, DEFAULT_BUFFER_SIZE));
             final int n = input.read(skipByteBuffer);
             if (n == EOF) {
                 break;
@@ -2177,18 +2215,11 @@ public static long skip(final Reader reader, final long toSkip) throws IOExcepti
         if (toSkip < 0) {
             throw new IllegalArgumentException("Skip count must be non-negative, actual: " + toSkip);
         }
-        /*
-         * N.B. no need to synchronize this because: - we don't care if the buffer is created multiple times (the data
-         * is ignored) - we always use the same size buffer, so if it it is recreated it will still be OK (if the buffer
-         * size were variable, we would need to synch. to ensure some other thread did not create a smaller one)
-         */
-        if (SKIP_CHAR_BUFFER == null) {
-            SKIP_CHAR_BUFFER = new char[SKIP_BYTE_BUFFER.length];
-        }
         long remain = toSkip;
         while (remain > 0) {
             // See https://issues.apache.org/jira/browse/IO-203 for why we use read() rather than delegating to skip()
-            final long n = reader.read(SKIP_CHAR_BUFFER, 0, (int) Math.min(remain, SKIP_BYTE_BUFFER.length));
+            final char[] charArray = getCharArray();
+            final long n = reader.read(charArray, 0, (int) Math.min(remain, charArray.length));
             if (n < 0) { // EOF
                 break;
             }