You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by bo...@apache.org on 2011/04/19 07:35:05 UTC
svn commit: r1094856 -
/commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
Author: bodewig
Date: Tue Apr 19 05:35:04 2011
New Revision: 1094856
URL: http://svn.apache.org/viewvc?rev=1094856&view=rev
Log:
print a warning if finalize closes the archive
Modified:
commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
Modified: commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
URL: http://svn.apache.org/viewvc/commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java?rev=1094856&r1=1094855&r2=1094856&view=diff
==============================================================================
--- commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java (original)
+++ commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java Tue Apr 19 05:35:04 2011
@@ -102,6 +102,11 @@ public class ZipFile {
private final ZipEncoding zipEncoding;
/**
+ * File name of actual source.
+ */
+ private final String archiveName;
+
+ /**
* The actual data source.
*/
private final RandomAccessFile archive;
@@ -180,6 +185,7 @@ public class ZipFile {
*/
public ZipFile(File f, String encoding, boolean useUnicodeExtraFields)
throws IOException {
+ this.archiveName = f.getAbsolutePath();
this.encoding = encoding;
this.zipEncoding = ZipEncodingHelper.getZipEncoding(encoding);
this.useUnicodeExtraFields = useUnicodeExtraFields;
@@ -214,7 +220,11 @@ public class ZipFile {
* @throws IOException if an error occurs closing the archive.
*/
public void close() throws IOException {
+ // this flag is only written here and read in finalize() which
+ // can never be run in parallel.
+ // no synchronization needed.
closed = true;
+
archive.close();
}
@@ -320,7 +330,11 @@ public class ZipFile {
*/
protected void finalize() throws Throwable {
try {
- close();
+ if (!closed) {
+ System.err.println("Cleaning up unclosed ZipFile for archive "
+ + archiveName);
+ close();
+ }
} finally {
super.finalize();
}
Re: svn commit: r1094856 - /commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
Posted by sebb <se...@gmail.com>.
On 19 April 2011 15:30, Stefan Bodewig <bo...@apache.org> wrote:
> On 2011-04-19, sebb wrote:
>
>> On 19 April 2011 06:35, <bo...@apache.org> wrote:
>
>>>> // this flag is only written here and read in finalize() which
>>>> // can never be run in parallel.
>>>> // no synchronization needed.
>
>> Are you sure?
>
> At least as long as finalize is not called by anybody else but the
> garbage collector.
>
> You can't call close without a reference to the object and as long as
> such a reference exists finalize will not be called.
OK, I was thinking that GC would be equivalent to a separate thread.
But thinking about it, I suppose the JMM must make special provision
for the GC to ensure that cached writes are flushed.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1094856 - /commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
Posted by Paul Benedict <pb...@apache.org>.
Sorry, 100% agreement with sebb. I read the attribution wrong :-)
On Tue, Apr 19, 2011 at 10:26 AM, Paul Benedict <pb...@apache.org>wrote:
> I carried my Effective Java 2nd Edition book in to work today.
>
> It's item #7. On Page 29 says, Josh says, "While there is no guarantee
> that the finalizer will be invoked promptly, it may be better to free the
> resource
> late than never, in those (hopefully rare) cases where the client fails to
> call
> the explicit termination method. But the finalizer should log a warning if
> it
> finds that the resource has not been terminated"
>
>
> On Tue, Apr 19, 2011 at 10:13 AM, sebb <se...@gmail.com> wrote:
>
>> On 19 April 2011 16:00, Torsten Curdt <tc...@vafer.org> wrote:
>> > I am really not comfortable doing all this stuff in finalize. Why use
>> > finalize at all?
>> > If someone forgot a close then he has to find and fix this in his code.
>> >
>> > Darn. Cannot find the reference I am thinking of why using "finalize"
>> > usually is really a bad idea. Was it from Bloch? Can't remember.
>>
>> Bloch does say that generally finalizers should not be used..
>>
>> However, he does say that they can be useful for "safety nets" in case
>> the object owner forgets to terminate it.
>> Better late than never.
>> In which case he says the finalizer should log a warning if the
>> resource has not been correctly terminated.
>>
>> This is exactly what we are doing here.
>>
>> > cheers,
>> > Torsten
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> > For additional commands, e-mail: dev-help@commons.apache.org
>> >
>> >
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
Re: svn commit: r1094856 - /commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
Posted by Paul Benedict <pb...@apache.org>.
100% agreement with Torsten.... I carried my Effective Java 2nd Edition book
in to work today.
It's item #7. On Page 29 says, Josh says, "While there is no guarantee
that the finalizer will be invoked promptly, it may be better to free the
resource
late than never, in those (hopefully rare) cases where the client fails to
call
the explicit termination method. But the finalizer should log a warning if
it
finds that the resource has not been terminated"
On Tue, Apr 19, 2011 at 10:13 AM, sebb <se...@gmail.com> wrote:
> On 19 April 2011 16:00, Torsten Curdt <tc...@vafer.org> wrote:
> > I am really not comfortable doing all this stuff in finalize. Why use
> > finalize at all?
> > If someone forgot a close then he has to find and fix this in his code.
> >
> > Darn. Cannot find the reference I am thinking of why using "finalize"
> > usually is really a bad idea. Was it from Bloch? Can't remember.
>
> Bloch does say that generally finalizers should not be used..
>
> However, he does say that they can be useful for "safety nets" in case
> the object owner forgets to terminate it.
> Better late than never.
> In which case he says the finalizer should log a warning if the
> resource has not been correctly terminated.
>
> This is exactly what we are doing here.
>
> > cheers,
> > Torsten
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: svn commit: r1094856 - /commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
Posted by sebb <se...@gmail.com>.
On 19 April 2011 16:00, Torsten Curdt <tc...@vafer.org> wrote:
> I am really not comfortable doing all this stuff in finalize. Why use
> finalize at all?
> If someone forgot a close then he has to find and fix this in his code.
>
> Darn. Cannot find the reference I am thinking of why using "finalize"
> usually is really a bad idea. Was it from Bloch? Can't remember.
Bloch does say that generally finalizers should not be used..
However, he does say that they can be useful for "safety nets" in case
the object owner forgets to terminate it.
Better late than never.
In which case he says the finalizer should log a warning if the
resource has not been correctly terminated.
This is exactly what we are doing here.
> cheers,
> Torsten
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1094856 - /commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
Posted by Torsten Curdt <tc...@vafer.org>.
I am really not comfortable doing all this stuff in finalize. Why use
finalize at all?
If someone forgot a close then he has to find and fix this in his code.
Darn. Cannot find the reference I am thinking of why using "finalize"
usually is really a bad idea. Was it from Bloch? Can't remember.
cheers,
Torsten
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1094856 - /commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
Posted by Stefan Bodewig <bo...@apache.org>.
On 2011-04-19, sebb wrote:
> On 19 April 2011 06:35, <bo...@apache.org> wrote:
>>> // this flag is only written here and read in finalize() which
>>> // can never be run in parallel.
>>> // no synchronization needed.
> Are you sure?
At least as long as finalize is not called by anybody else but the
garbage collector.
You can't call close without a reference to the object and as long as
such a reference exists finalize will not be called.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1094856 - /commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
Posted by sebb <se...@gmail.com>.
On 19 April 2011 06:35, <bo...@apache.org> wrote:
> Author: bodewig
> Date: Tue Apr 19 05:35:04 2011
> New Revision: 1094856
>
> URL: http://svn.apache.org/viewvc?rev=1094856&view=rev
> Log:
> print a warning if finalize closes the archive
>
> Modified:
> commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
>
> Modified: commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
> URL: http://svn.apache.org/viewvc/commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java?rev=1094856&r1=1094855&r2=1094856&view=diff
> ==============================================================================
> --- commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java (original)
> +++ commons/proper/compress/trunk/src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java Tue Apr 19 05:35:04 2011
> @@ -102,6 +102,11 @@ public class ZipFile {
> private final ZipEncoding zipEncoding;
>
> /**
> + * File name of actual source.
> + */
> + private final String archiveName;
> +
> + /**
> * The actual data source.
> */
> private final RandomAccessFile archive;
> @@ -180,6 +185,7 @@ public class ZipFile {
> */
> public ZipFile(File f, String encoding, boolean useUnicodeExtraFields)
> throws IOException {
> + this.archiveName = f.getAbsolutePath();
> this.encoding = encoding;
> this.zipEncoding = ZipEncodingHelper.getZipEncoding(encoding);
> this.useUnicodeExtraFields = useUnicodeExtraFields;
> @@ -214,7 +220,11 @@ public class ZipFile {
> * @throws IOException if an error occurs closing the archive.
> */
> public void close() throws IOException {
> + // this flag is only written here and read in finalize() which
> + // can never be run in parallel.
> + // no synchronization needed.
Are you sure?
If finalize() runs in a different thread from close(), then the field
should be volatile to ensure safe publication.
> closed = true;
> +
> archive.close();
> }
>
> @@ -320,7 +330,11 @@ public class ZipFile {
> */
> protected void finalize() throws Throwable {
> try {
> - close();
> + if (!closed) {
> + System.err.println("Cleaning up unclosed ZipFile for archive "
> + + archiveName);
> + close();
> + }
> } finally {
> super.finalize();
> }
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org