Fixing NPE because of EOFException.getMessage returning null

EOFException.getMessage returns null because of an inconsistency between readTrailer() and readUByte() when handling eof.
This change is meant to ignore this exception as the contents of the http response have actually been read and processed.


Details:
Please take a look at the stack trace of exception which is actually thrown before reading the analysis below:

What seems to be happening is:
toByteArray calls instream.read continously and stops when -1 is returned by instream.read
instream = EofSensorInputStream with wrappedstream as GZIPInputStream.read() method calls wrappedStream.read(byte)
FilteredInputStream.read(byte[]) calls this.read(byte[], 0, length)
GZIPInputStream.read(byte[], offset, len) returns -1 if end of stream is reached, otherwise it calls super.read(). This call returns -1
Now it calls readTrailer() and then is supposed to set end of stream reached = true

readTrailer() calls readUInt(in)   [ in = input stream being read, which in this case is org.apache.http.impl.io.ContentLengthInputStream ]
readUInt() calls readUShort
readUShort() calls readUByte
readUByte() calls in.read() which returns -1. readUByte checks if its -1 and throws EOFException with no message :( :(

This exception is caught by EofSensorInputStream.read() and it passes the exception along as is, without enclosing it in some other exception.
This is now caught by BasicHttpFetcher.toByteArraySafe() which checks if eofe.getMessage().equals("Unexpected end of ZLIB input stream") and this throws the NullPointerException because the message was never set by any1


What seems to be an obvious bug here is that readUByte does not want input stream to return -1, whereas readTrailer() is specifically called when the stream returns -1.
A bug has been filed with Sun and will update the code with the bug id once Sun has accepted the bug.


java.util.zip.GZIPInputStream.readUByte(GZIPInputStream.java:224)
java.util.zip.GZIPInputStream.readUShort(GZIPInputStream.java:214)
java.util.zip.GZIPInputStream.readUInt(GZIPInputStream.java:206)
java.util.zip.GZIPInputStream.readTrailer(GZIPInputStream.java:196)
java.util.zip.GZIPInputStream.read(GZIPInputStream.java:111)
java.io.FilterInputStream.read(FilterInputStream.java:107)
org.apache.http.conn.EofSensorInputStream.read(EofSensorInputStream.java:178)
org.apache.shindig.gadgets.http.BasicHttpFetcher.toByteArraySafe(BasicHttpFetcher.java:513)
org.apache.shindig.gadgets.http.BasicHttpFetcher.makeResponse(BasicHttpFetcher.java:469)
org.apache.shindig.gadgets.http.BasicHttpFetcher.fetch(BasicHttpFetcher.java:365)
com.google.gadgets.server.fetcher.RoutedHttpFetcher$RoutedHttpFetcherWrapper.fetch(RoutedHttpFetcher.java:34)
com.google.gadgets.server.tracker.ResourceUsageListener$TrackedFetcher.fetch(ResourceUsageListener.java:355)
com.google.gadgets.server.fetcher.RoutingHttpFetcher.fetch(RoutingHttpFetcher.java:54)
com.google.gadgets.server.fetcher.FilteredHttpFetcher.fetch(FilteredHttpFetcher.java:66)
com.google.gadgets.server.GoogleRequestPipeline.fetchNoCache(GoogleRequestPipeline.java:296)
com.google.gadgets.server.GoogleRequestPipeline.fetchWithCache(GoogleRequestPipeline.java:249)
com.google.gadgets.server.GoogleRequestPipeline.batchFetch(GoogleRequestPipeline.java:203)
com.google.gadgets.server.GoogleRequestPipeline.execute(GoogleRequestPipeline.java:142)
org.apache.shindig.gadgets.servlet.AccelHandler.doFetch(AccelHandler.java:84)
org.apache.shindig.gadgets.servlet.ProxyBase.fetch(ProxyBase.java:165)
org.apache.shindig.gadgets.servlet.HtmlAccelServlet.doGet(HtmlAccelServlet.java:63)

[vikaas.arora, 2010-07-22 05:00:53]

http://codereview.appspot.com/1872044/diff/3001/4001
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/1872044/diff/3001/4001#newcode525
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:525:
if ((instream.available() == 0) && eofe.getMessage() != null &&
Add this case to the unittest case added for this function.

http://codereview.appspot.com/1872044/diff/3001/4001#newcode528
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:528:
} else if (eofe.getMessage() == null) {
In case, exception message is not set, we have to re-throw the exception. We can
only digest the specific exception if the message type indicates the JDK bug.
Otherwise, we should be backward compatible.

Optionally, before re-throwing the exception, some default message can be added

[gagan.goku, 2010-07-24 03:44:20]

http://codereview.appspot.com/1872044/diff/3001/4001
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/1872044/diff/3001/4001#newcode525
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:525:
if ((instream.available() == 0) && eofe.getMessage() != null &&
On 2010/07/22 05:00:53, vikaas.arora wrote:
> Add this case to the unittest case added for this function.

Done.

http://codereview.appspot.com/1872044/diff/3001/4001#newcode528
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:528:
} else if (eofe.getMessage() == null) {
On 2010/07/22 05:00:53, vikaas.arora wrote:
> In case, exception message is not set, we have to re-throw the exception. We
can
> only digest the specific exception if the message type indicates the JDK bug.
> Otherwise, we should be backward compatible.
> 
> Optionally, before re-throwing the exception, some default message can be
added

Agreed with you on maintaining backward compatibility.
I have changed the description of this code review to reflect another bug that
might trouble us which is also because of GZIPInputStream.
Long term solution would be to get it fixed there itself, but for short term I
think we should fix the bugs we know of.
I will look into how to raise this bug appropriately with correct set of people.

[gagan.goku, 2010-07-24 04:29:51]

Filed a bug with Sun.

On Sat, Jul 24, 2010 at 9:14 AM, <gagan.goku@gmail.com> wrote:

>
> http://codereview.appspot.com/1872044/diff/3001/4001
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
> (right):
>
> http://codereview.appspot.com/1872044/diff/3001/4001#newcode525
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:525:
> if ((instream.available() == 0) && eofe.getMessage() != null &&
> On 2010/07/22 05:00:53, vikaas.arora wrote:
>
>> Add this case to the unittest case added for this function.
>>
>
> Done.
>
>
> http://codereview.appspot.com/1872044/diff/3001/4001#newcode528
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:528:
> } else if (eofe.getMessage() == null) {
> On 2010/07/22 05:00:53, vikaas.arora wrote:
>
>> In case, exception message is not set, we have to re-throw the
>>
> exception. We can
>
>> only digest the specific exception if the message type indicates the
>>
> JDK bug.
>
>> Otherwise, we should be backward compatible.
>>
>
>  Optionally, before re-throwing the exception, some default message can
>>
> be added
>
> Agreed with you on maintaining backward compatibility.
> I have changed the description of this code review to reflect another
> bug that might trouble us which is also because of GZIPInputStream.
> Long term solution would be to get it fixed there itself, but for short
> term I think we should fix the bugs we know of.
> I will look into how to raise this bug appropriately with correct set of
> people.
>
>
> http://codereview.appspot.com/1872044/show
>

[gagan.goku, 2010-07-26 05:29:27]

Filed a bug with Sun.


>
> On Sat, Jul 24, 2010 at 9:14 AM, <gagan.goku@gmail.com> wrote:
>
>>
>> http://codereview.appspot.com/1872044/diff/3001/4001
>> File
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
>> (right):
>>
>> http://codereview.appspot.com/1872044/diff/3001/4001#newcode525
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:525:
>> if ((instream.available() == 0) && eofe.getMessage() != null &&
>> On 2010/07/22 05:00:53, vikaas.arora wrote:
>>
>>> Add this case to the unittest case added for this function.
>>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/1872044/diff/3001/4001#newcode528
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:528:
>> } else if (eofe.getMessage() == null) {
>> On 2010/07/22 05:00:53, vikaas.arora wrote:
>>
>>> In case, exception message is not set, we have to re-throw the
>>>
>> exception. We can
>>
>>> only digest the specific exception if the message type indicates the
>>>
>> JDK bug.
>>
>>> Otherwise, we should be backward compatible.
>>>
>>
>>  Optionally, before re-throwing the exception, some default message can
>>>
>> be added
>>
>> Agreed with you on maintaining backward compatibility.
>> I have changed the description of this code review to reflect another
>> bug that might trouble us which is also because of GZIPInputStream.
>> Long term solution would be to get it fixed there itself, but for short
>> term I think we should fix the bugs we know of.
>> I will look into how to raise this bug appropriately with correct set of
>> people.
>>
>>
>> http://codereview.appspot.com/1872044/show
>>
>
>

[vikaas.arora, 2010-07-26 06:24:59]

http://codereview.appspot.com/1872044/diff/13001/5003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/1872044/diff/13001/5003#newcode517
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:517:
(eofe.getMessage() == null ||
Since now we are ignoring the exception even for the case where exception
message is not set (null), please change the comment block (below) to add
reference to the SUN bug that you have created and 1-2 lines about the nature of
the bug. That way the catch logic will be in sync with the comment.
Also you can move the comment block just before the 'if' condition.

[gagan.goku, 2010-07-26 11:47:08]

http://codereview.appspot.com/1872044/diff/13001/5003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/1872044/diff/13001/5003#newcode517
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:517:
(eofe.getMessage() == null ||
On 2010/07/26 06:24:59, vikaas.arora wrote:
> Since now we are ignoring the exception even for the case where exception
> message is not set (null), please change the comment block (below) to add
> reference to the SUN bug that you have created and 1-2 lines about the nature
of
> the bug. That way the catch logic will be in sync with the comment.
> Also you can move the comment block just before the 'if' condition.

I have filed the bug with with Sun but they need to accept it. I will update
this file once the bug is updated by Sun.
Let me add more information about the bug in this comment block.

[anupama.dutta, 2010-07-26 11:54:36]

LGTM

[johnfargo, 2010-07-26 22:15:23]

FYI committed last week, as r966159.

http://svn.apache.org/viewvc?view=revision&revision=966159

[gagan.goku, 2010-07-27 05:42:05]

On 2010/07/26 22:15:23, johnfargo wrote:
> FYI committed last week, as r966159.
> 
> http://svn.apache.org/viewvc?view=revision&revision=966159

Hi John

These are actually 2 related but different bugs. You have checked in Vikas's bug
which is a known GZIP inflater bug.
This bug is another bug in GZIPInputStream that we found while working on some
more test cases. The problem is that GZIPInputStream.readTrailer and
GZIPInputStream.readUByte have different expectations of what to do when end of
stream is reached.

In short, what happens is that we sometimes get an EOFException whose
getMessage() returns null.
I have mentioned a short and detailed analysis of the bug and will add the bug
id when Sun accepts the bug.

[johnfargo, 2010-07-27 16:36:35]

Missed that line. Reviewing the new delta.

On Mon, Jul 26, 2010 at 10:42 PM, <gagan.goku@gmail.com> wrote:

> On 2010/07/26 22:15:23, johnfargo wrote:
>
>> FYI committed last week, as r966159.
>>
>
>  http://svn.apache.org/viewvc?view=revision&revision=966159
>>
>
> Hi John
>
> These are actually 2 related but different bugs. You have checked in
> Vikas's bug which is a known GZIP inflater bug.
> This bug is another bug in GZIPInputStream that we found while working
> on some more test cases. The problem is that GZIPInputStream.readTrailer
> and GZIPInputStream.readUByte have different expectations of what to do
> when end of stream is reached.
>
> In short, what happens is that we sometimes get an EOFException whose
> getMessage() returns null.
> I have mentioned a short and detailed analysis of the bug and will add
> the bug id when Sun accepts the bug.
>
>
> http://codereview.appspot.com/1872044/show
>

[johnfargo, 2010-07-28 01:14:11]

LGTM, committed as r979922.

On Tue, Jul 27, 2010 at 9:36 AM, John Hjelmstad <johnfargo@gmail.com> wrote:

> Missed that line. Reviewing the new delta.
>
>
> On Mon, Jul 26, 2010 at 10:42 PM, <gagan.goku@gmail.com> wrote:
>
>> On 2010/07/26 22:15:23, johnfargo wrote:
>>
>>> FYI committed last week, as r966159.
>>>
>>
>>  http://svn.apache.org/viewvc?view=revision&revision=966159
>>>
>>
>> Hi John
>>
>> These are actually 2 related but different bugs. You have checked in
>> Vikas's bug which is a known GZIP inflater bug.
>> This bug is another bug in GZIPInputStream that we found while working
>> on some more test cases. The problem is that GZIPInputStream.readTrailer
>> and GZIPInputStream.readUByte have different expectations of what to do
>> when end of stream is reached.
>>
>> In short, what happens is that we sometimes get an EOFException whose
>> getMessage() returns null.
>> I have mentioned a short and detailed analysis of the bug and will add
>> the bug id when Sun accepts the bug.
>>
>>
>> http://codereview.appspot.com/1872044/show
>>
>
>