You are viewing a plain text version of this content. The canonical link for it is here.
Posted to general@xerces.apache.org by "George T. Joseph" <gt...@peakin.com> on 2000/02/07 08:30:40 UTC
Patch for ChunkyByteArray.java
Under 2 of 3 EOF conditions, the InputStream under ChunkyByteArray was not being
closed before fInputStream was set to null. This resulted in files being left
open even after all parsing was complete. Usually the JVM cleans this up when
the InputStream is garbage collected. However, if the GC doesn't run (either
because a large heap is allocated or because it's turned off) the files remain
locked.
george
*** xml-xerces/java/src/org/apache/xerces/utils/ChunkyByteArray.java Tue Jan 25
19:26:44 2000
--- src/org/apache/xerces/utils/ChunkyByteArray.java Mon Feb 07 02:07:16 2000
***************
*** 109,116 ****
int b = (int)(fData[0][fOffset]);
if (++fOffset == fLength) {
fData = null;
! if (fLength < CHUNK_SIZE)
! fInputStream = null;
}
return b;
}
--- 109,115 ----
int b = (int)(fData[0][fOffset]);
if (++fOffset == fLength) {
fData = null;
! if (fLength < CHUNK_SIZE) close();
}
return b;
}
***************
*** 134,141 ****
byte[] chunk = fData[0];
if (length >= bytesLeft) {
length = bytesLeft;
! if (fLength < CHUNK_SIZE)
! fInputStream = null;
}
if (buffer == null) {
fOffset += length;
--- 133,139 ----
byte[] chunk = fData[0];
if (length >= bytesLeft) {
length = bytesLeft;
! if (fLength < CHUNK_SIZE)close();
}
if (buffer == null) {
fOffset += length;
***************
*** 209,216 ****
result = fInputStream.read(data, offset, capacity);
if (result == -1) {
data[offset] = (byte)0xff;
! fInputStream.close();
! fInputStream = null;
break;
}
if (result > 0) {
--- 207,213 ----
result = fInputStream.read(data, offset, capacity);
if (result == -1) {
data[offset] = (byte)0xff;
! close();
break;
}
if (result > 0) {
***************
*** 220,225 ****
--- 217,230 ----
}
} while (capacity > 0);
}
+ public void close() throws IOException
+ {
+ if (fInputStream != null)
+ {
+ fInputStream.close();
+ fInputStream=null;
+ }
+ }
//
// Chunk size constants
//
RE: Patch for ChunkyByteArray.java
Posted by "George T. Joseph" <gt...@peakin.com>.
OK, the new patch is below. I left "fInputStream=null" in close(). There are a
few places in ChunkyByteArray where fInputStream is tested for null as an
indication of whether the stream has been closed. I think they're perfectly
valid since much of java.io does exactly the same thing.
george
*** xml-xerces/java/src/org/apache/xerces/utils/ChunkyByteArray.java Tue Jan 25
19:26:44 2000
--- src/org/apache/xerces/utils/ChunkyByteArray.java Mon Feb 07 22:13:52 2000
***************
*** 109,116 ****
int b = (int)(fData[0][fOffset]);
if (++fOffset == fLength) {
fData = null;
- if (fLength < CHUNK_SIZE)
- fInputStream = null;
}
return b;
}
--- 109,114 ----
***************
*** 134,141 ****
byte[] chunk = fData[0];
if (length >= bytesLeft) {
length = bytesLeft;
- if (fLength < CHUNK_SIZE)
- fInputStream = null;
}
if (buffer == null) {
fOffset += length;
--- 132,137 ----
***************
*** 209,216 ****
result = fInputStream.read(data, offset, capacity);
if (result == -1) {
data[offset] = (byte)0xff;
- fInputStream.close();
- fInputStream = null;
break;
}
if (result > 0) {
--- 205,210 ----
***************
*** 220,225 ****
--- 214,225 ----
}
} while (capacity > 0);
}
+ public void close() throws IOException
+ {
+ if (fInputStream == null) return;
+ fInputStream.close();
+ fInputStream=null;
+ }
//
// Chunk size constants
//
-----Original Message-----
From: Andy Clark [mailto:andyc@apache.org]
Sent: Monday, February 07, 2000 7:50 PM
To: xerces-dev@xml.apache.org
Subject: Re: Patch for ChunkyByteArray.java
"George T. Joseph" wrote:
> ChunkyByteArray didn't have a close() method so if
> ChunkyByteArray.close() were called (and it IS being
> called properly), it would be InputStream.close() that
> was actually invoked. That's an empty method and
> therefore a leak.
You are absolutely correct. This needs to be fixed.
> So, I'll remove the calls to close() in ChunkyByteArray,
> but the close() method itself needs to stay. Also, the
> existing "fInputStream = null" statements have
> to stay removed so close() can call fInputStream.close().
Exactly. In fact, the fInputStream field should never be
set to null. But we need to make sure that there is no
code that relies on the fact that the field is null in
order to operate correctly. We may be fine just removing
the assignments to null, though.
--
Andy Clark * IBM, JTC - Silicon Valley * andyc@apache.org
Re: Patch for ChunkyByteArray.java
Posted by Andy Clark <an...@apache.org>.
"George T. Joseph" wrote:
> ChunkyByteArray didn't have a close() method so if
> ChunkyByteArray.close() were called (and it IS being
> called properly), it would be InputStream.close() that
> was actually invoked. That's an empty method and
> therefore a leak.
You are absolutely correct. This needs to be fixed.
> So, I'll remove the calls to close() in ChunkyByteArray,
> but the close() method itself needs to stay. Also, the
> existing "fInputStream = null" statements have
> to stay removed so close() can call fInputStream.close().
Exactly. In fact, the fInputStream field should never be
set to null. But we need to make sure that there is no
code that relies on the fact that the field is null in
order to operate correctly. We may be fine just removing
the assignments to null, though.
--
Andy Clark * IBM, JTC - Silicon Valley * andyc@apache.org
RE: Patch for ChunkyByteArray.java
Posted by "George T. Joseph" <gt...@peakin.com>.
I think we're each half-right. How about a compromise...
Every descendant of InputStream in java.io closes the encapsulated
InputStream when it's own close method is invoked. You wouldn't expect the
following code snipped to leak, would you?
BufferedInputStream bis =
new BufferedInputStream(new FileInputStream("aaa"))
bis.close()
ChunkyByteArray didn't have a close() method so if ChunkyByteArray.close() were
called (and it IS being called properly), it would be InputStream.close() that
was actually invoked. That's an empty method and therefore a leak.
So, I'll remove the calls to close() in ChunkyByteArray, but the close() method
itself needs to stay. Also, the existing "fInputStream = null" statements have
to stay removed so close() can call fInputStream.close().
george
-----Original Message-----
From: Andy Clark [mailto:andyc@apache.org]
Sent: Monday, February 07, 2000 2:35 PM
To: xerces-dev@xml.apache.org
Subject: Re: Patch for ChunkyByteArray.java
"George T. Joseph" wrote:
>
> Under 2 of 3 EOF conditions, the InputStream under ChunkyByteArray was not
being
> closed before fInputStream was set to null. This resulted in files being left
> open even after all parsing was complete. Usually the JVM cleans this up
when
> the InputStream is garbage collected. However, if the GC doesn't run (either
> because a large heap is allocated or because it's turned off) the files remain
> locked.
I would disagree that the ChunkyByteArray is responsible for
closing the stream. My general rule is that if you create
the stream, you close it. Since the ChunkyByteArray doesn't
open the stream, it should not presume that it is allowed
to close it. In fact, I'm thinking that the one place where
ChunkyByteArray closes fInputStream should be removed.
However, this does bring up a very good point. Are there
places in the code where a stream is opened internal to the
parser, yet not closed? Patches for those conditions would be
gladly accepted! :)
--
Andy Clark * IBM, JTC - Silicon Valley * andyc@apache.org
Re: Patch for ChunkyByteArray.java
Posted by Andy Clark <an...@apache.org>.
"George T. Joseph" wrote:
>
> Under 2 of 3 EOF conditions, the InputStream under ChunkyByteArray was not being
> closed before fInputStream was set to null. This resulted in files being left
> open even after all parsing was complete. Usually the JVM cleans this up when
> the InputStream is garbage collected. However, if the GC doesn't run (either
> because a large heap is allocated or because it's turned off) the files remain
> locked.
I would disagree that the ChunkyByteArray is responsible for
closing the stream. My general rule is that if you create
the stream, you close it. Since the ChunkyByteArray doesn't
open the stream, it should not presume that it is allowed
to close it. In fact, I'm thinking that the one place where
ChunkyByteArray closes fInputStream should be removed.
However, this does bring up a very good point. Are there
places in the code where a stream is opened internal to the
parser, yet not closed? Patches for those conditions would be
gladly accepted! :)
--
Andy Clark * IBM, JTC - Silicon Valley * andyc@apache.org