Suspected regression: fix for 6735255 causes delay in GC of ZipFile InputStreams, increase in heap demand

[David Holmes]

Hi Neil,

Neil Richards said the following on 04/02/11 11:28:
> David, I can't claim to have reached true enlightenment wrt the section
> of the JLS that you point to - I suspect I'll need to lie down in a
> darkened room with a dampened flannel across the brow and ponder the
> nature of things a while to get there :-)
> 
> In the meantime, can you (or other knowledgeable GC folk) advise whether
> it has any implications which would cause the current suggested fix to
> be unsuitable?
> (To recap, the current suggestion has the ZipFileInflaterInputStream's
> close() method synchronized on itself, and called from its finalize()
> method. The close() method has both the read and write of the 'isClosed'
> field).
> 
> Any reassurance (or otherwise) gratefully accepted on this matter,

I don't fully understand the object relationships here so I'll just make 
a couple of observations on the synchronization in this code:

     public synchronized void close() throws IOException {
         if (!isClosed) {
             super.close();
             if (false == inFinalizer)
                 releaseInflater(inf);
             isClosed = true;
         }
     }

     public int available() throws IOException {
         if (isClosed)
             return 0;
         long avail = zfin.size() - inf.getBytesWritten();
         ...
     }

     protected void finalize() throws IOException {
         inFinalizer = true;
         close();
     }

1. If a call to close() occurs around the same time as finalization 
occurs then the finalizer thread will set inFinalizer to true, at which 
point the thread executing close() can see it is true and so may skip 
the releaseInflater(inf) call.

2. As isClosed is not volatile, and available() is not synchronized, a 
thread calling available() on a closed stream may not see that it has 
been closed and so will likely encounter an IOException rather than 
getting a zero return.


Even if #1 is not a practical problem I'd be inclined to make the 
finalizer synchronized as well. By doing that you're effectively 
ensuring that premature finalization of the stream won't happen.

For #2 it is a tricky call. If you don't actually expect the stream 
object to be used by multiple threads then using a synchronized block to 
read isClosed will be cheap in VMs that go to great effort to reduce the 
cost of unnecessary synchronization (eg biased-locking in hotspot). 
Otherwise making isClosed volatile is likely the better option.

HTH

David