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

[Neil Richards]

On Sat, 2011-04-02 at 09:17 +1000, David Holmes wrote:
> Xueming Shen said the following on 04/02/11 05:07:
> > On 04/01/2011 09:42 AM, Neil Richards wrote:
> >> On Wed, 2011-03-30 at 13:31 -0700, Xueming Shen wrote:
> >>> Isn't it true that when the finalize()->close() gets invoked, there
> >>> should be no strong reference anywhere else that you can use to invoke
> >>> close() in other thread?
> >> It's true that once finalize() has been called, there can't be another
> >> explicit call to close().
> >> However, if close() is explicitly called first, it will be called again
> >> when finalize() calls it, so one still wants the update to 'isClosed' to
> >> be seen by the finalizer thread (in this case).
> > 
> > I'm not a GC guy, so I might be missing something here, but if close() 
> > is being explicitly invoked by some thread, means someone has a
> > strong reference to it, I don't think the finalize() can kick in
> > until that close() returns and the strong reference used to make
> > that explicit invocation is cleared. The InputStream is eligible
> > for finalization only after it is "weakly" reachable, means no more
> > "stronger" reachable exists, right?
> 
> Actually no. One of the more obscure corner cases with finalization is 
> that you can actually finalize an object that is still being used. The 
> JLS actually spells this out - see section 12.6.1 and in particular the 
> Discussion within that section.

Also, I don't think my argument rests just on the behaviour in this
corner case.

Consider a normal thread with a normal (ie. "strong") reference
explicitly calls close(), and so updates the 'isClosed' field to 'true'.
To guarantee that that update is flushed from that normal thread's local
memory cache down to main memory, and to guarantee another thread (eg.
the finalizer thread) will see the update on a subsequent call (by
fetching the value from main memory rather than relying on its local
memory cache), one must either:
     1. Using some form of synchronization for both the write and the
        read of the field's value.
     2. Mark the field as being 'volatile'.

As the logic inside close() involving 'isClosed' is intended to only let
the first caller through to perform the close operations, it seems more
appropriate in this case to use synchronization here.


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,
Thanks,
Neil

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU