On 03/30/2011 12:53 PM, Neil Richards wrote:
Hi Sherman,
Thanks for your review and comments on this.
On Tue, 2011-03-29 at 12:05 -0700, Xueming Shen wrote:
Hi Neil,
It appears to be a "regression" in scenario you described (user
application never close the input stream after use and the ZipFile
instance being retained during the lifetime of the process). The
proposed approach seems to solve this particular problem.
Here are my comments regarding the patch.
(1) ZipFileInflaterInputStream.close() now synchronizes on
Zipfile.this, is it really necessary?
Synchronization is used so that an update to 'isClosed' on one thread is
seen by another.
Adding finalization (ie. a call from finalize() to close()) effectively
introduces a new thread into the mix, namely the finalization thread.
Whilst you're correct in saying that, in general, InputStream gives no
thread-safe guarantees, in this particular case I believe it is valid to
make sure updates to this field are seen across threads, to prevent the
logic of close() being run twice.
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?
ZipFileInputStream class has to sync on ZipFile.this, the read/close()
methods are
accessing the underlying/shared bits of the ZipFile. For
ZipFileInflaterInputStream.close()
case, even we want to make sure the isClose is synced, I would prefer
to see a "local"
lock, maybe simply make close() synchronized is better?
-Sherman
I chose to synchronize on ZipFile.this merely for consistency - ie.
because that's what ZipFileInputStream (the other InputStream impl in
ZipFile) does.
I suppose I felt the risks of creating some kind of obscure deadlock
scenario was minimised by copying the existing pattern.
I can be easily swayed if you think there's a better object to
synchronize on, that doesn't run an increased risk of deadlock.
(2) Is there any particular reason that we don't want to "reuse" the
Inflater(), if the close comes from the finalize() in your testing
evn? One thing we noticed before is that the moment the
InputStream gets finalized, the Inflater embedded might have been
finalized already, this is the reason why we have a "inf.ended()"
check in releaseInflater().
The ZipFileInflaterInputStream object's finalize() method is called by
the finalization thread, after it has been added to the finalization
queue by GC.
If *it* has been added to the finalization queue (because it's eligible
for GC), then so will the Inflater object it is referencing.
As finalization gives no guarantees as to the order in which objects are
finalized, regardless of whether finalize() has been called on the
Inflater object, it is nevertheless the case that its method *will* be
called at some point, so the Inflater object *will* be ended.
If it has been put back into the inflater cache (because ZFIIS's
finalize happened to be called first), then this ended inflater will lie
in wait for another ZFIIS to use it, which would result in a
particularly unexpected exception being thrown.
In general, I believe it to be quite frowned on to "resurrect" objects
which are going through finalization (including those queued up for
finalization) and place them back into the live set of object, as weird
behaviour often ensues if one does.
(Especially as, iirc, finalize() is only called once per object,
regardless of whether it is "dug back up" in this fashion).
(3) The attached test case does "f.close()" in its finally block, I
guess it should be f.delete()?
When I wrote the testcase, I relied upon File.deleteOnExit() to take
care of cleaning up the temporary file(s) created.
However, I recall now that I don't think it guarantees deletion on
abnormal termination.
I agree that calling f.delete() instead of f.close() would be a good
improvement to the testcase.
Please find below an updated changeset, with this change made and the
bug number filled in.
Hope this helps to clarify things,
Thanks again,
Neil
