You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Tim Ellison <t....@gmail.com> on 2009/11/26 22:18:03 UTC
[classlib][archive] problems with ZipFile 9was: Re: [testing] M12
testing on Windows x86)
On 26/Nov/2009 18:44, Tim Ellison wrote:
> On 26/Nov/2009 17:00, Oliver Deakin wrote:
>> A little more progress.
>> It looks like we're reading javax\swing\text\html\parser\html32.bdtd
>> (which is where the ZipFile/ZipEntry classes come in). I see that with
>> the M12 version of those classes we end up throwing an ASN1Exception at
>> line 886 of BerInputStream.java (right at the start of it's read()
>> method). This unravels the stack up to ParserDelegator.createDTD() which
>> has a "try ... catch(Exception e) { // do nothing }" around the DTD
>> read. The exception is ignored and we carry on, but the DTD has not been
>> fully populated and it looks like this is the cause of the html not
>> being processed correctly and the tests failing. Needless to say, this
>> exception is not thrown with the M11 version of ZipFile/ZipEntry.
>>
>> The question now is why offset==buffer.length at the start of the
>> BerInputStream.read() call with the new ZipFile/ZipEntry classes. Ill
>> keep digging.
>
> I'll bet you a beer that it is our friend available() again...
>
> Consider this:
>
> static final String NAME = "javax/swing/text/html/parser/html32.bdtd";
>
> public void test() throws IOException {
> JarFile jar = new JarFile("swing.jar");
> ZipEntry ze = jar.getEntry(NAME);
> InputStream is = jar.getInputStream(ze);
>
> System.out.println("Size = " + ze.getSize());
> System.out.println("Available = " + is.available());
>
> jar.close();
> }
>
> On Harmony it prints:
> Size = 51140
> Available = 1
>
> On the RI it prints:
> Size = 51140
> Available = 51140
>
> I know the arguments that say available() should not be used to judge
> the total number of bytes that can be read, but it seems that a number
> of applications (including our generated parser?) use it in this way.
>
> I haven't tried changing it yet[1], but I'll take that bet if you want :-)
>
> Regards,
> Tim
>
> [1] I'm supposed to be packing for a long weekend away, so I'll be quiet
> for the next few days.
Ok, so I may owe you a beer :-)
A quick trial of implementing available() for the inflater stream fixes
this test case (above) but not the original problem.
Given there are these differences we see emerging, I wonder if we should
revert the offending code and retry again after M12?
FYI here is my hacked available() impl, which needs some testing before
it is good to go in..
Index: src/main/java/java/util/zip/ZipFile.java
===================================================================
--- src/main/java/java/util/zip/ZipFile.java (revision 884543)
+++ src/main/java/java/util/zip/ZipFile.java (working copy)
@@ -247,7 +247,7 @@
rafstrm.skip(entry.nameLen + localExtraLenOrWhatever);
rafstrm.mLength = rafstrm.mOffset + entry.compressedSize;
if (entry.compressionMethod == ZipEntry.DEFLATED) {
- return new InflaterInputStream(rafstrm, new
Inflater(true));
+ return new ZipInflaterInputStream(rafstrm, new
Inflater(true), 1024, entry);
} else {
return rafstrm;
}
@@ -415,4 +415,27 @@
return n;
}
}
+
+ static class ZipInflaterInputStream extends InflaterInputStream {
+
+ ZipEntry entry;
+ long bytesRead = 0;
+
+ public ZipInflaterInputStream(InputStream is, Inflater inf, int
bsize, ZipEntry entry) {
+ super(is, inf, bsize);
+ this.entry = entry;
+ }
+
+ public int read(byte[] buffer, int off, int nbytes) throws
IOException {
+ int i = super.read(buffer, off, nbytes);
+ if (i != -1) {
+ bytesRead += i;
+ }
+ return i;
+ }
+
+ public int available() throws IOException {
+ return super.available() == 0 ? 0 : (int) (entry.getSize()
- bytesRead);
+ }
+ }
}
Re: [classlib][archive] problems with ZipFile 9was: Re: [testing] M12
testing on Windows x86)
Posted by enh <en...@google.com>.
Correct.
On Nov 27, 2009 1:10 PM, "Nathan Beyer" <nd...@apache.org> wrote:
On Fri, Nov 27, 2009 at 5:30 AM, Oliver Deakin <ol...@googlemail.com>
wrote: > > > Tim Ellis...
Isn't this still incorrect? The available() method isn't equivalent to
what's left in the stream - it's just what's available before the read
will start blocking. Don't you have to read from the stream until it
hits EOF/EOS?
If the use case requires that the entire stream must be read before
moving on, then there's really no point in using available() at all.
Correct?
-Nathan
-Nathan
> Regards, > Oliver > > >> Index: src/main/java/java/util/zip/ZipFile.java
>> =====================...
Re: [classlib][archive] problems with ZipFile 9was: Re: [testing]
M12 testing on Windows x86)
Posted by Oliver Deakin <ol...@googlemail.com>.
enh wrote:
> may as well fix the lot, rather than sniff these out one by one...
>
Agreed, although we cannot fix them now for M12. Could you log these in
a JIRA so we have a record of them please?
Regards,
Oliver
> ./rmi/src/main/java/java/rmi/server/RMIClassLoader.java:214:
> byte[] buf = new byte[in.available()];
> ./luni/src/main/java/org/apache/harmony/luni/util/InputStreamExposer.java:86:
> buffer = new byte[available];
> ./luni/src/main/java/org/apache/harmony/luni/util/ExposedByteArrayInputStream.java:52:
> final byte[] buffer = new byte[available];
> ./luni/src/test/api/common/org/apache/harmony/luni/tests/java/io/SerializationStressTest.java:670:
> byte[] input = new byte[dis.available()];
> ./print/src/test/api/java/common/javax/print/ValueTests.java:562:
> res = new byte [iStream.available()];
> ./security/src/main/java/common/org/apache/harmony/security/utils/JarUtils.java:147:
> byte[] sfBytes = new byte[signature.available()];
> ./swing/src/main/java/common/javax/swing/plaf/basic/BasicLookAndFeel.java:1022:
> byte[] data = new
> byte[audioStream.available()];
> ./swing/src/main/java/common/javax/swing/text/html/parser/DTD.java:145:
> byte[] enc = new byte[stream.available()];
>
> --elliott
>
> On Thu, Nov 26, 2009 at 13:18, Tim Ellison <t....@gmail.com> wrote:
>
>> On 26/Nov/2009 18:44, Tim Ellison wrote:
>>
>>> On 26/Nov/2009 17:00, Oliver Deakin wrote:
>>>
>>>> A little more progress.
>>>> It looks like we're reading javax\swing\text\html\parser\html32.bdtd
>>>> (which is where the ZipFile/ZipEntry classes come in). I see that with
>>>> the M12 version of those classes we end up throwing an ASN1Exception at
>>>> line 886 of BerInputStream.java (right at the start of it's read()
>>>> method). This unravels the stack up to ParserDelegator.createDTD() which
>>>> has a "try ... catch(Exception e) { // do nothing }" around the DTD
>>>> read. The exception is ignored and we carry on, but the DTD has not been
>>>> fully populated and it looks like this is the cause of the html not
>>>> being processed correctly and the tests failing. Needless to say, this
>>>> exception is not thrown with the M11 version of ZipFile/ZipEntry.
>>>>
>>>> The question now is why offset==buffer.length at the start of the
>>>> BerInputStream.read() call with the new ZipFile/ZipEntry classes. Ill
>>>> keep digging.
>>>>
>>> I'll bet you a beer that it is our friend available() again...
>>>
>>> Consider this:
>>>
>>> static final String NAME = "javax/swing/text/html/parser/html32.bdtd";
>>>
>>> public void test() throws IOException {
>>> JarFile jar = new JarFile("swing.jar");
>>> ZipEntry ze = jar.getEntry(NAME);
>>> InputStream is = jar.getInputStream(ze);
>>>
>>> System.out.println("Size = " + ze.getSize());
>>> System.out.println("Available = " + is.available());
>>>
>>> jar.close();
>>> }
>>>
>>> On Harmony it prints:
>>> Size = 51140
>>> Available = 1
>>>
>>> On the RI it prints:
>>> Size = 51140
>>> Available = 51140
>>>
>>> I know the arguments that say available() should not be used to judge
>>> the total number of bytes that can be read, but it seems that a number
>>> of applications (including our generated parser?) use it in this way.
>>>
>>> I haven't tried changing it yet[1], but I'll take that bet if you want :-)
>>>
>>> Regards,
>>> Tim
>>>
>>> [1] I'm supposed to be packing for a long weekend away, so I'll be quiet
>>> for the next few days.
>>>
>> Ok, so I may owe you a beer :-)
>>
>> A quick trial of implementing available() for the inflater stream fixes
>> this test case (above) but not the original problem.
>>
>> Given there are these differences we see emerging, I wonder if we should
>> revert the offending code and retry again after M12?
>>
>> FYI here is my hacked available() impl, which needs some testing before
>> it is good to go in..
>>
>> Index: src/main/java/java/util/zip/ZipFile.java
>> ===================================================================
>> --- src/main/java/java/util/zip/ZipFile.java (revision 884543)
>> +++ src/main/java/java/util/zip/ZipFile.java (working copy)
>> @@ -247,7 +247,7 @@
>> rafstrm.skip(entry.nameLen + localExtraLenOrWhatever);
>> rafstrm.mLength = rafstrm.mOffset + entry.compressedSize;
>> if (entry.compressionMethod == ZipEntry.DEFLATED) {
>> - return new InflaterInputStream(rafstrm, new
>> Inflater(true));
>> + return new ZipInflaterInputStream(rafstrm, new
>> Inflater(true), 1024, entry);
>> } else {
>> return rafstrm;
>> }
>> @@ -415,4 +415,27 @@
>> return n;
>> }
>> }
>> +
>> + static class ZipInflaterInputStream extends InflaterInputStream {
>> +
>> + ZipEntry entry;
>> + long bytesRead = 0;
>> +
>> + public ZipInflaterInputStream(InputStream is, Inflater inf, int
>> bsize, ZipEntry entry) {
>> + super(is, inf, bsize);
>> + this.entry = entry;
>> + }
>> +
>> + public int read(byte[] buffer, int off, int nbytes) throws
>> IOException {
>> + int i = super.read(buffer, off, nbytes);
>> + if (i != -1) {
>> + bytesRead += i;
>> + }
>> + return i;
>> + }
>> +
>> + public int available() throws IOException {
>> + return super.available() == 0 ? 0 : (int) (entry.getSize()
>> - bytesRead);
>> + }
>> + }
>> }
>>
>>
>>
>>
>
>
>
>
--
Oliver Deakin
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Re: [classlib][archive] problems with ZipFile 9was: Re: [testing] M12
testing on Windows x86)
Posted by enh <en...@google.com>.
may as well fix the lot, rather than sniff these out one by one...
./rmi/src/main/java/java/rmi/server/RMIClassLoader.java:214:
byte[] buf = new byte[in.available()];
./luni/src/main/java/org/apache/harmony/luni/util/InputStreamExposer.java:86:
buffer = new byte[available];
./luni/src/main/java/org/apache/harmony/luni/util/ExposedByteArrayInputStream.java:52:
final byte[] buffer = new byte[available];
./luni/src/test/api/common/org/apache/harmony/luni/tests/java/io/SerializationStressTest.java:670:
byte[] input = new byte[dis.available()];
./print/src/test/api/java/common/javax/print/ValueTests.java:562:
res = new byte [iStream.available()];
./security/src/main/java/common/org/apache/harmony/security/utils/JarUtils.java:147:
byte[] sfBytes = new byte[signature.available()];
./swing/src/main/java/common/javax/swing/plaf/basic/BasicLookAndFeel.java:1022:
byte[] data = new
byte[audioStream.available()];
./swing/src/main/java/common/javax/swing/text/html/parser/DTD.java:145:
byte[] enc = new byte[stream.available()];
--elliott
On Thu, Nov 26, 2009 at 13:18, Tim Ellison <t....@gmail.com> wrote:
> On 26/Nov/2009 18:44, Tim Ellison wrote:
>> On 26/Nov/2009 17:00, Oliver Deakin wrote:
>>> A little more progress.
>>> It looks like we're reading javax\swing\text\html\parser\html32.bdtd
>>> (which is where the ZipFile/ZipEntry classes come in). I see that with
>>> the M12 version of those classes we end up throwing an ASN1Exception at
>>> line 886 of BerInputStream.java (right at the start of it's read()
>>> method). This unravels the stack up to ParserDelegator.createDTD() which
>>> has a "try ... catch(Exception e) { // do nothing }" around the DTD
>>> read. The exception is ignored and we carry on, but the DTD has not been
>>> fully populated and it looks like this is the cause of the html not
>>> being processed correctly and the tests failing. Needless to say, this
>>> exception is not thrown with the M11 version of ZipFile/ZipEntry.
>>>
>>> The question now is why offset==buffer.length at the start of the
>>> BerInputStream.read() call with the new ZipFile/ZipEntry classes. Ill
>>> keep digging.
>>
>> I'll bet you a beer that it is our friend available() again...
>>
>> Consider this:
>>
>> static final String NAME = "javax/swing/text/html/parser/html32.bdtd";
>>
>> public void test() throws IOException {
>> JarFile jar = new JarFile("swing.jar");
>> ZipEntry ze = jar.getEntry(NAME);
>> InputStream is = jar.getInputStream(ze);
>>
>> System.out.println("Size = " + ze.getSize());
>> System.out.println("Available = " + is.available());
>>
>> jar.close();
>> }
>>
>> On Harmony it prints:
>> Size = 51140
>> Available = 1
>>
>> On the RI it prints:
>> Size = 51140
>> Available = 51140
>>
>> I know the arguments that say available() should not be used to judge
>> the total number of bytes that can be read, but it seems that a number
>> of applications (including our generated parser?) use it in this way.
>>
>> I haven't tried changing it yet[1], but I'll take that bet if you want :-)
>>
>> Regards,
>> Tim
>>
>> [1] I'm supposed to be packing for a long weekend away, so I'll be quiet
>> for the next few days.
>
> Ok, so I may owe you a beer :-)
>
> A quick trial of implementing available() for the inflater stream fixes
> this test case (above) but not the original problem.
>
> Given there are these differences we see emerging, I wonder if we should
> revert the offending code and retry again after M12?
>
> FYI here is my hacked available() impl, which needs some testing before
> it is good to go in..
>
> Index: src/main/java/java/util/zip/ZipFile.java
> ===================================================================
> --- src/main/java/java/util/zip/ZipFile.java (revision 884543)
> +++ src/main/java/java/util/zip/ZipFile.java (working copy)
> @@ -247,7 +247,7 @@
> rafstrm.skip(entry.nameLen + localExtraLenOrWhatever);
> rafstrm.mLength = rafstrm.mOffset + entry.compressedSize;
> if (entry.compressionMethod == ZipEntry.DEFLATED) {
> - return new InflaterInputStream(rafstrm, new
> Inflater(true));
> + return new ZipInflaterInputStream(rafstrm, new
> Inflater(true), 1024, entry);
> } else {
> return rafstrm;
> }
> @@ -415,4 +415,27 @@
> return n;
> }
> }
> +
> + static class ZipInflaterInputStream extends InflaterInputStream {
> +
> + ZipEntry entry;
> + long bytesRead = 0;
> +
> + public ZipInflaterInputStream(InputStream is, Inflater inf, int
> bsize, ZipEntry entry) {
> + super(is, inf, bsize);
> + this.entry = entry;
> + }
> +
> + public int read(byte[] buffer, int off, int nbytes) throws
> IOException {
> + int i = super.read(buffer, off, nbytes);
> + if (i != -1) {
> + bytesRead += i;
> + }
> + return i;
> + }
> +
> + public int available() throws IOException {
> + return super.available() == 0 ? 0 : (int) (entry.getSize()
> - bytesRead);
> + }
> + }
> }
>
>
>
--
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Re: [classlib][archive] problems with ZipFile 9was: Re: [testing] M12
testing on Windows x86)
Posted by Nathan Beyer <nd...@apache.org>.
On Fri, Nov 27, 2009 at 5:30 AM, Oliver Deakin
<ol...@googlemail.com> wrote:
>
>
> Tim Ellison wrote:
>>
>> <snip>
>> FYI here is my hacked available() impl, which needs some testing before
>> it is good to go in..
>>
>
> With this patch for available() applied the ASN1Exception no longer gets
> thrown in BerInputStream.read(), but instead an ASN1Exception is thrown in
> BerInputStream.readSequence() at line 671. The message from this exception
> is "Mandatory value is missing at [3255]".
>
> Looking in the byte buffer contents, I see that after index 3255 all its
> values are 0, whereas in M11 they are all populated as expected. It looks
> like DTD.read() is assuming that its stream.read(enc) call (around line 149)
> will return a full buffer, but with the new zip changes it does not.
> Applying a patch something like this (along with Tim's ZipFile patch below)
> fixes the test failures and I get a clean run of the full swing test suite:
>
> Index: DTD.java
> ===================================================================
> --- DTD.java (revision 884214)
> +++ DTD.java (working copy)
> @@ -142,8 +142,12 @@
>
> public void read(final DataInputStream stream) throws IOException {
> // converts from DataInputStream into a byte array
> - byte[] enc = new byte[stream.available()];
> - stream.read(enc);
> + int available = stream.available();
> + byte[] enc = new byte[available];
> + int readBytes = 0;
> + while (readBytes != available) {
> + readBytes += stream.read(enc, readBytes, available -
> readBytes);
> + }
> // decode the byte array
> Asn1Dtd asn1 = new Asn1Dtd(enc);
>
Isn't this still incorrect? The available() method isn't equivalent to
what's left in the stream - it's just what's available before the read
will start blocking. Don't you have to read from the stream until it
hits EOF/EOS?
If the use case requires that the entire stream must be read before
moving on, then there's really no point in using available() at all.
Correct?
-Nathan
-Nathan
> Regards,
> Oliver
>
>
>> Index: src/main/java/java/util/zip/ZipFile.java
>> ===================================================================
>> --- src/main/java/java/util/zip/ZipFile.java (revision 884543)
>> +++ src/main/java/java/util/zip/ZipFile.java (working copy)
>> @@ -247,7 +247,7 @@
>> rafstrm.skip(entry.nameLen + localExtraLenOrWhatever);
>> rafstrm.mLength = rafstrm.mOffset + entry.compressedSize;
>> if (entry.compressionMethod == ZipEntry.DEFLATED) {
>> - return new InflaterInputStream(rafstrm, new
>> Inflater(true));
>> + return new ZipInflaterInputStream(rafstrm, new
>> Inflater(true), 1024, entry);
>> } else {
>> return rafstrm;
>> }
>> @@ -415,4 +415,27 @@
>> return n;
>> }
>> }
>> +
>> + static class ZipInflaterInputStream extends InflaterInputStream {
>> +
>> + ZipEntry entry;
>> + long bytesRead = 0;
>> +
>> + public ZipInflaterInputStream(InputStream is, Inflater inf, int
>> bsize, ZipEntry entry) {
>> + super(is, inf, bsize);
>> + this.entry = entry;
>> + }
>> +
>> + public int read(byte[] buffer, int off, int nbytes) throws
>> IOException {
>> + int i = super.read(buffer, off, nbytes);
>> + if (i != -1) {
>> + bytesRead += i;
>> + }
>> + return i;
>> + }
>> +
>> + public int available() throws IOException {
>> + return super.available() == 0 ? 0 : (int) (entry.getSize()
>> - bytesRead);
>> + }
>> + }
>> }
>>
>>
>>
>>
>
> --
> Oliver Deakin
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number
> 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire
> PO6 3AU
>
>
Re: [classlib][archive] problems with ZipFile
Posted by Tim Ellison <t....@gmail.com>.
On 30/Nov/2009 10:32, Tim Ellison wrote:
> On 27/Nov/2009 11:30, Oliver Deakin wrote:
>> Tim Ellison wrote:
>>> <snip>
>>> FYI here is my hacked available() impl, which needs some testing before
>>> it is good to go in..
>>>
>> With this patch for available() applied the ASN1Exception no longer gets
>> thrown in BerInputStream.read(), but instead an ASN1Exception is thrown
>> in BerInputStream.readSequence() at line 671. The message from this
>> exception is "Mandatory value is missing at [3255]".
>>
>> Looking in the byte buffer contents, I see that after index 3255 all its
>> values are 0, whereas in M11 they are all populated as expected. It
>> looks like DTD.read() is assuming that its stream.read(enc) call (around
>> line 149) will return a full buffer, but with the new zip changes it
>> does not. Applying a patch something like this (along with Tim's ZipFile
>> patch below) fixes the test failures and I get a clean run of the full
>> swing test suite:
>>
>> Index: DTD.java
>> ===================================================================
>> --- DTD.java (revision 884214)
>> +++ DTD.java (working copy)
>> @@ -142,8 +142,12 @@
>>
>> public void read(final DataInputStream stream) throws IOException {
>> // converts from DataInputStream into a byte array
>> - byte[] enc = new byte[stream.available()];
>> - stream.read(enc);
>> + int available = stream.available();
>> + byte[] enc = new byte[available];
>> + int readBytes = 0;
>> + while (readBytes != available) {
>> + readBytes += stream.read(enc, readBytes, available -
>> readBytes);
>> + }
>> // decode the byte array
>> Asn1Dtd asn1 = new Asn1Dtd(enc);
>
>
>
> While I agree that using available is wrong (just wanted to say that in
> every mail!), consider...
>
>
> JarFile jar = new JarFile("swing.jar");
> ZipEntry ze = jar.getEntry(NAME);
> InputStream is = jar.getInputStream(ze);
>
> is.available();
> System.out.println("Size = " + ze.getSize());
>
> while (is.available() > 0) {
> int avail = is.available();
> int i = is.read(new byte[avail]);
> System.out.printf(
> "Bytes requested = %d, got = %d, remaining = %d\n",
> avail, i, is.available());
> }
> jar.close();
>
>
> On harmony (with patches) it prints:
> Size = 51140
> Bytes requested = 51140, got = 3256, remaining = 47884
> Bytes requested = 47884, got = 5072, remaining = 42812
> Bytes requested = 42812, got = 19083, remaining = 23729
> Bytes requested = 23729, got = 17749, remaining = 5980
> Bytes requested = 5980, got = 5980, remaining = 0
>
>
> On the RI it prints:
> Size = 51140
> Bytes requested = 51140, got = 51140, remaining = 0
The Harmony behavior is courtesy of me choosing a random 1024 byte
working buffer in the ZipInflaterInputStream hack. We can get the data
in fewer reads by increasing it. I updated my patch and attached it to
HARMONY-6394.
Regards,
Tim
Re: [classlib][archive] problems with ZipFile
Posted by Tim Ellison <t....@gmail.com>.
On 27/Nov/2009 11:30, Oliver Deakin wrote:
> Tim Ellison wrote:
>> <snip>
>> FYI here is my hacked available() impl, which needs some testing before
>> it is good to go in..
>>
>
> With this patch for available() applied the ASN1Exception no longer gets
> thrown in BerInputStream.read(), but instead an ASN1Exception is thrown
> in BerInputStream.readSequence() at line 671. The message from this
> exception is "Mandatory value is missing at [3255]".
>
> Looking in the byte buffer contents, I see that after index 3255 all its
> values are 0, whereas in M11 they are all populated as expected. It
> looks like DTD.read() is assuming that its stream.read(enc) call (around
> line 149) will return a full buffer, but with the new zip changes it
> does not. Applying a patch something like this (along with Tim's ZipFile
> patch below) fixes the test failures and I get a clean run of the full
> swing test suite:
>
> Index: DTD.java
> ===================================================================
> --- DTD.java (revision 884214)
> +++ DTD.java (working copy)
> @@ -142,8 +142,12 @@
>
> public void read(final DataInputStream stream) throws IOException {
> // converts from DataInputStream into a byte array
> - byte[] enc = new byte[stream.available()];
> - stream.read(enc);
> + int available = stream.available();
> + byte[] enc = new byte[available];
> + int readBytes = 0;
> + while (readBytes != available) {
> + readBytes += stream.read(enc, readBytes, available -
> readBytes);
> + }
> // decode the byte array
> Asn1Dtd asn1 = new Asn1Dtd(enc);
While I agree that using available is wrong (just wanted to say that in
every mail!), consider...
JarFile jar = new JarFile("swing.jar");
ZipEntry ze = jar.getEntry(NAME);
InputStream is = jar.getInputStream(ze);
is.available();
System.out.println("Size = " + ze.getSize());
while (is.available() > 0) {
int avail = is.available();
int i = is.read(new byte[avail]);
System.out.printf(
"Bytes requested = %d, got = %d, remaining = %d\n",
avail, i, is.available());
}
jar.close();
On harmony (with patches) it prints:
Size = 51140
Bytes requested = 51140, got = 3256, remaining = 47884
Bytes requested = 47884, got = 5072, remaining = 42812
Bytes requested = 42812, got = 19083, remaining = 23729
Bytes requested = 23729, got = 17749, remaining = 5980
Bytes requested = 5980, got = 5980, remaining = 0
On the RI it prints:
Size = 51140
Bytes requested = 51140, got = 51140, remaining = 0
Regards,
Tim
Re: [classlib][archive] problems with ZipFile 9was: Re: [testing]
M12 testing on Windows x86)
Posted by Oliver Deakin <ol...@googlemail.com>.
Tim Ellison wrote:
> <snip>
> FYI here is my hacked available() impl, which needs some testing before
> it is good to go in..
>
With this patch for available() applied the ASN1Exception no longer gets
thrown in BerInputStream.read(), but instead an ASN1Exception is thrown
in BerInputStream.readSequence() at line 671. The message from this
exception is "Mandatory value is missing at [3255]".
Looking in the byte buffer contents, I see that after index 3255 all its
values are 0, whereas in M11 they are all populated as expected. It
looks like DTD.read() is assuming that its stream.read(enc) call (around
line 149) will return a full buffer, but with the new zip changes it
does not. Applying a patch something like this (along with Tim's ZipFile
patch below) fixes the test failures and I get a clean run of the full
swing test suite:
Index: DTD.java
===================================================================
--- DTD.java (revision 884214)
+++ DTD.java (working copy)
@@ -142,8 +142,12 @@
public void read(final DataInputStream stream) throws IOException {
// converts from DataInputStream into a byte array
- byte[] enc = new byte[stream.available()];
- stream.read(enc);
+ int available = stream.available();
+ byte[] enc = new byte[available];
+ int readBytes = 0;
+ while (readBytes != available) {
+ readBytes += stream.read(enc, readBytes, available -
readBytes);
+ }
// decode the byte array
Asn1Dtd asn1 = new Asn1Dtd(enc);
Regards,
Oliver
> Index: src/main/java/java/util/zip/ZipFile.java
> ===================================================================
> --- src/main/java/java/util/zip/ZipFile.java (revision 884543)
> +++ src/main/java/java/util/zip/ZipFile.java (working copy)
> @@ -247,7 +247,7 @@
> rafstrm.skip(entry.nameLen + localExtraLenOrWhatever);
> rafstrm.mLength = rafstrm.mOffset + entry.compressedSize;
> if (entry.compressionMethod == ZipEntry.DEFLATED) {
> - return new InflaterInputStream(rafstrm, new
> Inflater(true));
> + return new ZipInflaterInputStream(rafstrm, new
> Inflater(true), 1024, entry);
> } else {
> return rafstrm;
> }
> @@ -415,4 +415,27 @@
> return n;
> }
> }
> +
> + static class ZipInflaterInputStream extends InflaterInputStream {
> +
> + ZipEntry entry;
> + long bytesRead = 0;
> +
> + public ZipInflaterInputStream(InputStream is, Inflater inf, int
> bsize, ZipEntry entry) {
> + super(is, inf, bsize);
> + this.entry = entry;
> + }
> +
> + public int read(byte[] buffer, int off, int nbytes) throws
> IOException {
> + int i = super.read(buffer, off, nbytes);
> + if (i != -1) {
> + bytesRead += i;
> + }
> + return i;
> + }
> +
> + public int available() throws IOException {
> + return super.available() == 0 ? 0 : (int) (entry.getSize()
> - bytesRead);
> + }
> + }
> }
>
>
>
>
--
Oliver Deakin
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Re: [classlib][archive] problems with ZipFile 9was: Re: [testing]
M12 testing on Windows x86)
Posted by Oliver Deakin <ol...@googlemail.com>.
Tim Ellison wrote:
> On 26/Nov/2009 18:44, Tim Ellison wrote:
>
>> On 26/Nov/2009 17:00, Oliver Deakin wrote:
>>
>>> <snip>
>>>
>> I'll bet you a beer that it is our friend available() again...
>>
>> Consider this:
>>
>> static final String NAME = "javax/swing/text/html/parser/html32.bdtd";
>>
>> public void test() throws IOException {
>> JarFile jar = new JarFile("swing.jar");
>> ZipEntry ze = jar.getEntry(NAME);
>> InputStream is = jar.getInputStream(ze);
>>
>> System.out.println("Size = " + ze.getSize());
>> System.out.println("Available = " + is.available());
>>
>> jar.close();
>> }
>>
>> On Harmony it prints:
>> Size = 51140
>> Available = 1
>>
>> On the RI it prints:
>> Size = 51140
>> Available = 51140
>>
>> I know the arguments that say available() should not be used to judge
>> the total number of bytes that can be read, but it seems that a number
>> of applications (including our generated parser?) use it in this way.
>>
>> I haven't tried changing it yet[1], but I'll take that bet if you want :-)
>>
>> Regards,
>> Tim
>>
>> [1] I'm supposed to be packing for a long weekend away, so I'll be quiet
>> for the next few days.
>>
>
> Ok, so I may owe you a beer :-)
>
Sounds good!
> A quick trial of implementing available() for the inflater stream fixes
> this test case (above) but not the original problem.
>
Yep, I see the same thing. Although (as Jesse mentioned in the JIRA) we
call available() in DTD.java and the results are significantly different
for M11 and M12, as exposed by your test case above, implementing
available() still does not fix the swing test case.
> Given there are these differences we see emerging, I wonder if we should
> revert the offending code and retry again after M12?
>
It would be tempting to do this so we can get M12 out the door and then
fix this issue for M13.
> FYI here is my hacked available() impl, which needs some testing before
> it is good to go in..
>
> Index: src/main/java/java/util/zip/ZipFile.java
> ===================================================================
> --- src/main/java/java/util/zip/ZipFile.java (revision 884543)
> +++ src/main/java/java/util/zip/ZipFile.java (working copy)
> @@ -247,7 +247,7 @@
> rafstrm.skip(entry.nameLen + localExtraLenOrWhatever);
> rafstrm.mLength = rafstrm.mOffset + entry.compressedSize;
> if (entry.compressionMethod == ZipEntry.DEFLATED) {
> - return new InflaterInputStream(rafstrm, new
> Inflater(true));
> + return new ZipInflaterInputStream(rafstrm, new
> Inflater(true), 1024, entry);
> } else {
> return rafstrm;
> }
> @@ -415,4 +415,27 @@
> return n;
> }
> }
> +
> + static class ZipInflaterInputStream extends InflaterInputStream {
> +
> + ZipEntry entry;
> + long bytesRead = 0;
> +
> + public ZipInflaterInputStream(InputStream is, Inflater inf, int
> bsize, ZipEntry entry) {
> + super(is, inf, bsize);
> + this.entry = entry;
> + }
> +
> + public int read(byte[] buffer, int off, int nbytes) throws
> IOException {
> + int i = super.read(buffer, off, nbytes);
> + if (i != -1) {
> + bytesRead += i;
> + }
> + return i;
> + }
> +
> + public int available() throws IOException {
> + return super.available() == 0 ? 0 : (int) (entry.getSize()
> - bytesRead);
> + }
> + }
> }
>
>
>
>
--
Oliver Deakin
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU