Fix weird 'EOF Exception' coming from JDK ZLIB input Stream

Due to JDK bug (Ref: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4040920), the 'EOF Exception' comes while accessing zip content. This issue is very inconsistent and happens for some zip resources and that too once in 4-5 times.

This is little hard to reproduce but when it comes, it comes for wrong case/reason, even though the resource itself is fetched fine. Refer the bug link, mentioned above.

The bug lies in the JDK 'Inflater.finished()' call, that erroneously returns 'false' even if the input stream (zip) is done reading the response. Whenever this happens, 'java.util.zip.InflaterInputStream.fill' will throw the 'EOFException'.

As a fix for this specific corner case, ignore the Exception (EOF with specific cause trail) and let pass all other exceptions.

I verified this working OK on the site for which this error was initially reported.

The change is trivial. I am getting soem issues in writing the appropriate Unit Test case for the same. Once the change looks OK to be submitted, will invest more time in writing the Unit Testcase for the same.

[gagan.goku, 2010-07-15 13:01:18]

http://codereview.appspot.com/1854041/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/1854041/diff/1/2#newcode502
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:502:
} catch (EOFException eofe) {
cant we simply enclose the call to EntityUtils.toByteArray like you have done
here and call it a day ?

[gagan.goku, 2010-07-15 13:04:17]

On 2010/07/15 13:01:18, gagan.goku wrote:
> http://codereview.appspot.com/1854041/diff/1/2
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
> (right):
> 
> http://codereview.appspot.com/1854041/diff/1/2#newcode502
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:502:
> } catch (EOFException eofe) {
> cant we simply enclose the call to EntityUtils.toByteArray like you have done
> here and call it a day ?

I meant enclose in try .. catch block

[vikaas.arora, 2010-07-15 14:25:17]

http://codereview.appspot.com/1854041/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/1854041/diff/1/2#newcode502
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:502:
} catch (EOFException eofe) {
On 2010/07/15 13:01:18, gagan.goku wrote:
> cant we simply enclose the call to EntityUtils.toByteArray like you have done
> here and call it a day ?

You meant in the original code (line#461 of this file).
Yes, you can intercept the Exception there, but how will you get the
responseBytes ?
byte[] responseBytes = null;
try {
 responseBytes = (entity == null) ? null : EntityUtils.toByteArray(ent  ity);
} catch (EOFException eofe) {
 responseBytes = ?  
}

Alternatively, one can also change the 'EntityUtils' class with this small
patch, but that apache module 'org.apache.http.util.EntityUtils' is out of bound
for us.

[gagan.goku, 2010-07-15 19:04:50]

http://codereview.appspot.com/1854041/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/1854041/diff/1/2#newcode502
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:502:
} catch (EOFException eofe) {
On 2010/07/15 14:25:17, vikaas.arora wrote:
> On 2010/07/15 13:01:18, gagan.goku wrote:
> > cant we simply enclose the call to EntityUtils.toByteArray like you have
done
> > here and call it a day ?
> 
> You meant in the original code (line#461 of this file).
> Yes, you can intercept the Exception there, but how will you get the
> responseBytes ?
> byte[] responseBytes = null;
> try {
>  responseBytes = (entity == null) ? null : EntityUtils.toByteArray(ent  ity);
> } catch (EOFException eofe) {
>  responseBytes = ?  
> }
> 
> Alternatively, one can also change the 'EntityUtils' class with this small
> patch, but that apache module 'org.apache.http.util.EntityUtils' is out of
bound
> for us.

Ahh, sorry about that. I guess this change makes sense, whenever EntityUtils bug
is fixed, this method can be reverted.
I guess it makes sense to not use our brain and just make this a replica of the
original method. Just 1 small nitpick though, please format in our coding style
(correct spaces etc..)
Looks good to me.

@John: Your thoughts ?

[vikaas.arora, 2010-07-16 12:44:58]

Fixed space formatting

[anupama.dutta, 2010-07-19 02:50:53]

http://codereview.appspot.com/1854041/diff/8001/9001
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(left):

http://codereview.appspot.com/1854041/diff/8001/9001#oldcode461
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:461:
byte[] responseBytes = (entity == null) ? null :
EntityUtils.toByteArray(entity);
I think we should keep the check for entity == null, and allow toByteArraySafe
to throw the usual IllegalArgumentException - this will ensure that if and when
the EOF bug is fixed in the original EntityUtils code, we can just replace the
toByteArraySafe call.

[vikaas.arora, 2010-07-19 03:24:23]

http://codereview.appspot.com/1854041/diff/8001/9001
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(left):

http://codereview.appspot.com/1854041/diff/8001/9001#oldcode461
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:461:
byte[] responseBytes = (entity == null) ? null :
EntityUtils.toByteArray(entity);
On 2010/07/19 02:50:53, anupama.dutta wrote:
> I think we should keep the check for entity == null, and allow toByteArraySafe
> to throw the usual IllegalArgumentException - this will ensure that if and
when
> the EOF bug is fixed in the original EntityUtils code, we can just replace the
> toByteArraySafe call.

The proposed code is in parity with the old code w.r.t entity=null. Earlier
'responseBytes' is set to null in the main function. In the new implementation,
the function 'toByteArraySafe' will return 'null' if entity is null. Even though
the old function 'EntityUtils.toByteArray' will throw the illegalArg Exception
(if called with null entity), the way it was called from this code, will not
execute that path. The old function was public and could be called from multiple
code paths. The new one is 'private' and it's called in a very controlled
execution path.

Once the EOF bug is fixed (this bug has been in JDK since 1998), we can call
'EntityUtils.toByteArray' from within new function 'toByteArraySafe' (under
'entity != null' check).

[vikaas.arora, 2010-07-19 03:45:56]

Added John & Ziv to reviewers. Retained the '(entity == null)' check in method
'makeResponse'

[anupama.dutta, 2010-07-19 05:12:35]

http://codereview.appspot.com/1854041/diff/14001/15001
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/1854041/diff/14001/15001#newcode482
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:482:
if (entity != null) {
To reduce indentation, you could say if (entity == null) { return responseBytes;
}
and then continue with the rest of the code as in
http://www.docjar.com/html/api/org/apache/http/util/EntityUtils.java.html

[vikaas.arora, 2010-07-19 06:36:32]

Anupama's feedback

[vikaas.arora, 2010-07-19 06:37:51]

http://codereview.appspot.com/1854041/diff/14001/15001
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/1854041/diff/14001/15001#newcode482
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:482:
if (entity != null) {
On 2010/07/19 05:12:36, anupama.dutta wrote:
> To reduce indentation, you could say if (entity == null) { return
responseBytes;
> }
> and then continue with the rest of the code as in
> http://www.docjar.com/html/api/org/apache/http/util/EntityUtils.java.html

Done.

[johnfargo, 2010-07-19 17:34:01]

Hi Vikas:

Very tricky, and strange, situation! The fix seems reasonable to me. My two
comments are just conservative programming suggestions/questions. Thoughts
welcome.

Cheers,
John

http://codereview.appspot.com/1854041/diff/18001/19001
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/1854041/diff/18001/19001#newcode480
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:480:
private byte[] toByteArraySafe(final HttpEntity entity) throws IOException {
Is there a reliable test we can do in this method to circumscribe the fix only
to the potential condition in which failure occurs, ie. when retrieving
ZLIB-inflated content?

For instance, entity.getContent() instanceof InflaterInputStream? I don't know
the implementation well enough to say whether that's a sensible solution.

http://codereview.appspot.com/1854041/diff/18001/19001#newcode513
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:513:
if (!eofe.getMessage().equals("Unexpected end of ZLIB input stream")) {
alternately (or in addition), per your comment:
"The bug lies in the JDK 'Inflater.finished()' call, that erroneously
returns 'false' even if the input stream (zip) is done reading the
response. Whenever this happens,
'java.util.zip.InflaterInputStream.fill' will throw the 'EOFException'."

...it sounds like you could pull "int l" out of the try-block and only swallow
the EOFException if l == -1 (ie. if the InputStream is finished reading).

[vikaas.arora, 2010-07-20 13:30:29]

http://codereview.appspot.com/1854041/diff/18001/19001
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/1854041/diff/18001/19001#newcode480
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:480:
private byte[] toByteArraySafe(final HttpEntity entity) throws IOException {
On 2010/07/19 17:34:02, johnfargo wrote:
> Is there a reliable test we can do in this method to circumscribe the fix only
> to the potential condition in which failure occurs, ie. when retrieving
> ZLIB-inflated content?
> 
> For instance, entity.getContent() instanceof InflaterInputStream? I don't know
> the implementation well enough to say whether that's a sensible solution.

I have changed this method to public and tried add a Unit testcase for it's
internal state. Having some trouble in mocking 'instream'. Will work on the Unit
test, in case the core logic is LGTM'ed.

http://codereview.appspot.com/1854041/diff/18001/19001#newcode513
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:513:
if (!eofe.getMessage().equals("Unexpected end of ZLIB input stream")) {
On 2010/07/19 17:34:02, johnfargo wrote:
> alternately (or in addition), per your comment:
> "The bug lies in the JDK 'Inflater.finished()' call, that erroneously
> returns 'false' even if the input stream (zip) is done reading the
> response. Whenever this happens,
> 'java.util.zip.InflaterInputStream.fill' will throw the 'EOFException'."
> 
> ...it sounds like you could pull "int l" out of the try-block and only swallow
> the EOFException if l == -1 (ie. if the InputStream is finished reading).

It's little complicated than the comment I had. Following is the debug traces:

- instream has a wrappedStream that's of type 'GZIPInputStream'.
- 'GZIPInputStream' follows the following stream class hierarchy:
   GZIPInputStream -> InflaterInputStream -> FilterInputStream -> InputStream
- For the case where this issue is exhibiting, the zipped stream length
(instream->wrappedStream->in->contentLength is 3999
- Deep down in the call hierarchy from here, there's code in
ContentLengthInputStream.read(), that reads data chunk in 512 buffer/array and
maintains the internal 'pos' pointer and make sure it doesn't skip past the
contentLength mark (in this case it's 3999. Any attempt to read past this mark
returns '-1' from this function.
- In-between the ContentLengthInputStream.read() and this function, there's
InflaterStream.read(), that has a magical function 'finished()' to deduce,
whether subsequent calls to 'ContentLengthInputStream.read()' are required or
not. If 'finished()' indicates EOF, this function returns '-1' (via call chains
of diff read() fns) to this function. If 'finished()' says false, it calls the
'ContentLengthInputStream.read()' and reads the raw content, unzip that content
in the buffer 'tmp' and returns the lenght of the unziped data (saved in local
var l).

So in normal flow, this 3999 raw data is read in 10 iterations viz a sequence of
10, 512, 512, 37, 512, 512, 512, 512, 512, 368; that's translated to inflated
data stream of 1071, 888, 215, 2581, ..., 3442, 2101 Bytes (saved to local var
l).
Past this the subsequent invocation to 'instream.read(tmp)' is intercepted by
'InflaterStream.read()' and magically 'finished()' method returns true and the
call flow is halted there with a return of '-1' to this master function that
exists from the while loop.
In the erroneous case (one in 5-6 iterations), the data has been read and
inflated and the last call, that should have been intercepted by
'InflaterStream.finished()=true', passes by this check and hits
'ContentLengthInputStream.read()', that returns '-1'. Now there's check in
'InflaterStream.fill()' that '-1' and throws the EOFException.

Now we can't add a check in this code and assume any last valid value for local
var 'l'. 

I found another signal that can be used here to restrict the check for not
throwing exception. The 'instream' has a method 'available()' whose document
(http://download.oracle.com/docs/cd/E17476_01/javase/1.4.2/docs/api/java/io/In...)
suggests that this returns the number of bytes that can be read. But my
observation was that this method was returning '1' for intermediate iterations
and '0' in the terminal iteration, where this Exception is thrown.

[vikaas.arora, 2010-07-20 13:32:55]

Addressed Fargo's comment (UT under way though)

[johnfargo, 2010-07-20 20:19:26]

Vikas:

Thanks for the thorough explanation, and the accommodation of your
observations based on this. Let me know when the UT is complete; I'll be
happy to review and hopefully commit in short order.

--j

On Tue, Jul 20, 2010 at 6:32 AM, <vikaas.arora@gmail.com> wrote:

> Addressed Fargo's comment (UT under way though)
>
>
> http://codereview.appspot.com/1854041/show
>

[vikaas.arora, 2010-07-21 06:49:10]

Added a unit test

[vikaas.arora, 2010-07-21 07:10:33]

unit Test

[johnfargo, 2010-07-21 09:55:18]

Looks good, Vikas. I'm still a little leery of disambiguating this use case
based on the String of its error condition, but I understand the rationale (and
the worst that could happen is that this particular fix doesn't trigger, which
is status quo).

Committed!

On 2010/07/21 07:10:33, vikaas.arora wrote:
> unit Test

[vikaas.arora, 2010-07-21 10:10:49]

Thanks for committing the fix

Given the corner case and inconsistent exception trigger, the check that we had
was very stringent. As you mentioned, if the conditions that are set are not
violated and still there's an issue (exception), worst case the
exception will again
be thrown. So in any case, it won't be worst than the existing behavior.

-Vikas

On Wed, Jul 21, 2010 at 3:25 PM,  <johnfargo@gmail.com> wrote:
> Looks good, Vikas. I'm still a little leery of disambiguating this use
> case based on the String of its error condition, but I understand the
> rationale (and the worst that could happen is that this particular fix
> doesn't trigger, which is status quo).
>
> Committed!
>
> On 2010/07/21 07:10:33, vikaas.arora wrote:
>>
>> unit Test
>
>
>
> http://codereview.appspot.com/1854041/show
>