Could somebody please take a look a the patch below and see if they
believe my explanation for why it fixes a findbugs problem in InputStream?
The problem: We have a static, skipBuf, that we only use to read junk
data from the stream when skipping a number of bytes. The static is
lazily initialized. It may be null, or too small for the job. The
problem is that another thread could also see null (or a small buffer)
and then set the buffer too small after the calling thread sets it to
/their/ required size.
The solution: If we take a local copy, localBuf, as a snapshot of
skipBuf before testing it, then even if the skipBuf value is slammed by
another thread we have our local copy. We can also blindly overwrite
any value stored by other threads since they will be doing the same thing.
So the update of the static is still unsafe, but the usage of it is ok
-- and this is better than making it volatile.
Agree?
Tim
Index: InputStream.java
===================================================================
--- InputStream.java (revision 820251)
+++ InputStream.java (working copy)
@@ -211,11 +211,16 @@
}
long skipped = 0;
int toRead = n < 4096 ? (int) n : 4096;
- if (skipBuf == null || skipBuf.length < toRead) {
- skipBuf = new byte[toRead];
+ // We are unsynchronized, so take a local copy of the skipBuf
at some
+ // point in time.
+ byte[] localBuf = skipBuf;
+ if (localBuf == null || localBuf.length < toRead) {
+ // May be lazily written back to the static. No matter if it
+ // overwrites somebody else's store.
+ skipBuf = localBuf = new byte[toRead];
}
while (skipped < n) {
- int read = read(skipBuf, 0, toRead);
+ int read = read(localBuf, 0, toRead);
if (read == -1) {
return skipped;
}
