issue 235: Leak in NetHttpResponse

issue 235: Leak in NetHttpResponse

[yanivi, 2013-07-17 22:58:14]

https://codereview.appspot.com/11472043/diff/4001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java
(right):

https://codereview.appspot.com/11472043/diff/4001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java:74:
public InputStream getContent() throws IOException {
write a unit test please

https://codereview.appspot.com/11472043/diff/4001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java:76:
return responseCode < 400 ? connection.getInputStream() :
connection.getErrorStream();
what evidence do we have that this the correct logic?

[peleyal, 2013-07-18 14:55:37]

https://codereview.appspot.com/11472043/diff/4001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java
(right):

https://codereview.appspot.com/11472043/diff/4001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java:74:
public InputStream getContent() throws IOException {
On 2013/07/17 22:58:15, yanivi wrote:
> write a unit test please

Done.

https://codereview.appspot.com/11472043/diff/4001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java:76:
return responseCode < 400 ? connection.getInputStream() :
connection.getErrorStream();
Take a look in the documentation of getInputStream and getErrorStream:
http://docs.oracle.com/javase/6/docs/api/java/net/HttpURLConnection.html#getE...
and
http://docs.oracle.com/javase/6/docs/api/java/net/URLConnection.html#getInput...

It doesn't say exactly, but based on the code of getErrorStream:

1674    @Override
1675    public InputStream More ...getErrorStream() {
1676        if (connected && responseCode >= 400) {
1677            // Client Error 4xx and Server Error 5xx
1678            if (errorStream != null) {
1679                return errorStream;
1680            } else if (inputStream != null) {
1681                return inputStream;
1682            }
1683        }
1684        return null;
1685    }

that error stream is returned for response code which is equal or higher to 400.
 On 2013/07/17 22:58:15, yanivi wrote:
> what evidence do we have that this the correct logic?

[yanivi, 2013-07-18 17:17:21]

https://codereview.appspot.com/11472043/diff/4001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java
(right):

https://codereview.appspot.com/11472043/diff/4001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java:76:
return responseCode < 400 ? connection.getInputStream() :
connection.getErrorStream();
On 2013/07/18 14:55:37, peleyal wrote:
> Take a look in the documentation of getInputStream and getErrorStream:
>
http://docs.oracle.com/javase/6/docs/api/java/net/HttpURLConnection.html#getE...)
> and
>
http://docs.oracle.com/javase/6/docs/api/java/net/URLConnection.html#getInput...)
> 
> It doesn't say exactly, but based on the code of getErrorStream:
> 
> 1674    @Override
> 1675    public InputStream More ...getErrorStream() {
> 1676        if (connected && responseCode >= 400) {
> 1677            // Client Error 4xx and Server Error 5xx
> 1678            if (errorStream != null) {
> 1679                return errorStream;
> 1680            } else if (inputStream != null) {
> 1681                return inputStream;
> 1682            }
> 1683        }
> 1684        return null;
> 1685    }
> 
> that error stream is returned for response code which is equal or higher to
400.
>  On 2013/07/17 22:58:15, yanivi wrote:
> > what evidence do we have that this the correct logic?
> 

The danger is that we may be relying on an implementation detail that is true
for one JDK implementation but may not be true for another.  that said, at least
with your changes we know it will work correctly for at least 1 implementation.

just to throw another possibility out there: what about doing something like:

InputStream errorStream = connection.getErrorStream();
return errorStream == null ? connection.getInputStream() : errorStream;

i haven't tried it myself, but I thought maybe it can be something you may want
to try.

also, please try this on Android, including older SDKs if you can.  you may even
try writing an android unit test which literally runs on an android device.

[peleyal, 2013-07-19 15:41:25]

https://codereview.appspot.com/11472043/diff/4001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java
(right):

https://codereview.appspot.com/11472043/diff/4001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java:76:
return responseCode < 400 ? connection.getInputStream() :
connection.getErrorStream();
1. The android documentation is the following:
If the HTTP response indicates that an error occurred, getInputStream() will
throw an IOException. Use getErrorStream() to read the error response. The
headers can be read in the normal way using getHeaderFields(),

2. The JDK code for getInputStream includes the following:
catch (IOException e) {
            rememberedException = e;

            // buffer the error stream if bytes < 4k
            // and it can be buffered within 1 second
            String te = responses.findValue("Transfer-Encoding");
            if (http != null && http.isKeepingAlive() && enableESBuffer &&
                (cl > 0 || (te != null && te.equalsIgnoreCase("chunked")))) {
                errorStream = ErrorStream.getErrorStream(inputStream, cl, http);
            }
            throw e;
        } 
That's the one and only place errorStream is set.
That's the reason for my change :)

On 2013/07/18 17:17:21, yanivi wrote:
> On 2013/07/18 14:55:37, peleyal wrote:
> > Take a look in the documentation of getInputStream and getErrorStream:
> >
>
http://docs.oracle.com/javase/6/docs/api/java/net/HttpURLConnection.html#getE...)
> > and
> >
>
http://docs.oracle.com/javase/6/docs/api/java/net/URLConnection.html#getInput...)
> > 
> > It doesn't say exactly, but based on the code of getErrorStream:
> > 
> > 1674    @Override
> > 1675    public InputStream More ...getErrorStream() {
> > 1676        if (connected && responseCode >= 400) {
> > 1677            // Client Error 4xx and Server Error 5xx
> > 1678            if (errorStream != null) {
> > 1679                return errorStream;
> > 1680            } else if (inputStream != null) {
> > 1681                return inputStream;
> > 1682            }
> > 1683        }
> > 1684        return null;
> > 1685    }
> > 
> > that error stream is returned for response code which is equal or higher to
> 400.
> >  On 2013/07/17 22:58:15, yanivi wrote:
> > > what evidence do we have that this the correct logic?
> > 
> 
> The danger is that we may be relying on an implementation detail that is true
> for one JDK implementation but may not be true for another.  that said, at
least
> with your changes we know it will work correctly for at least 1
implementation.
> 
> just to throw another possibility out there: what about doing something like:
> 
> InputStream errorStream = connection.getErrorStream();
> return errorStream == null ? connection.getInputStream() : errorStream;
> 
> i haven't tried it myself, but I thought maybe it can be something you may
want
> to try.
> 
> also, please try this on Android, including older SDKs if you can.  you may
even
> try writing an android unit test which literally runs on an android device.

[yanivi, 2013-07-22 12:32:44]

https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java
(right):

https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java:76:
HttpURLConnection connection = this.connection;
while you are here, remove this line to simplify the code

https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java:80:
return connection.getErrorStream();
might we be eating the IOException in case of some other networking problem?  or
would getErrorStream() then rethrow that same IOException?  I noticed
"rememberedException".

By the way, can you please also look at the Android open source code?

[peleyal, 2013-07-23 00:28:08]

https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java
(right):

https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java:76:
HttpURLConnection connection = this.connection;
On 2013/07/22 12:32:44, yanivi wrote:
> while you are here, remove this line to simplify the code

Done.

https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java:80:
return connection.getErrorStream();
where can I find it?
I added code that won't eat the IOException in the other cases.
On 2013/07/22 12:32:44, yanivi wrote:
> might we be eating the IOException in case of some other networking problem? 
or
> would getErrorStream() then rethrow that same IOException?  I noticed
> "rememberedException".
> 
> By the way, can you please also look at the Android open source code?

[yanivi, 2013-07-23 02:01:27]

https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java
(right):

https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java:80:
return connection.getErrorStream();
On 2013/07/23 00:28:08, peleyal wrote:
> where can I find it?

http://source.android.com/source/initializing.html

> I added code that won't eat the IOException in the other cases.

did you verify from the implementation if this is the right logic?

> On 2013/07/22 12:32:44, yanivi wrote:
> > might we be eating the IOException in case of some other networking problem?

> or
> > would getErrorStream() then rethrow that same IOException?  I noticed
> > "rememberedException".
> > 
> > By the way, can you please also look at the Android open source code?
>

[peleyal, 2013-07-23 02:02:40]

The sdk implementation. I don't know where to find the android sources
On Jul 22, 2013 10:01 PM, <yanivi@google.com> wrote:

>
> https://codereview.appspot.**com/11472043/diff/14001/**
> google-http-client/src/main/**java/com/google/api/client/**
>
http/javanet/NetHttpResponse.**java<https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java>
> File
> google-http-client/src/main/**java/com/google/api/client/**
> http/javanet/NetHttpResponse.**java
> (right):
>
> https://codereview.appspot.**com/11472043/diff/14001/**
> google-http-client/src/main/**java/com/google/api/client/**
>
http/javanet/NetHttpResponse.**java#newcode80<https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java#newcode80>
> google-http-client/src/main/**java/com/google/api/client/**
> http/javanet/NetHttpResponse.**java:80:
> return connection.getErrorStream();
> On 2013/07/23 00:28:08, peleyal wrote:
>
>> where can I find it?
>>
>
>
http://source.android.com/**source/initializing.html<http://source.android.co...
>
>  I added code that won't eat the IOException in the other cases.
>>
>
> did you verify from the implementation if this is the right logic?
>
>  On 2013/07/22 12:32:44, yanivi wrote:
>> > might we be eating the IOException in case of some other networking
>>
> problem?
>
>> or
>> > would getErrorStream() then rethrow that same IOException?  I
>>
> noticed
>
>> > "rememberedException".
>> >
>> > By the way, can you please also look at the Android open source
>>
> code?
>
>
>
https://codereview.appspot.**com/11472043/<https://codereview.appspot.com/114...
>

[yanivi, 2013-07-24 20:51:06]

On 2013/07/23 02:02:40, peleyal wrote:
> The sdk implementation. I don't know where to find the android sources
> On Jul 22, 2013 10:01 PM, <mailto:yanivi@google.com> wrote:
> 
> >
> > https://codereview.appspot.**com/11472043/diff/14001/**
> > google-http-client/src/main/**java/com/google/api/client/**
> >
>
http/javanet/NetHttpResponse.**java<https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java>
> > File
> > google-http-client/src/main/**java/com/google/api/client/**
> > http/javanet/NetHttpResponse.**java
> > (right):
> >
> > https://codereview.appspot.**com/11472043/diff/14001/**
> > google-http-client/src/main/**java/com/google/api/client/**
> >
>
http/javanet/NetHttpResponse.**java#newcode80<https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java#newcode80>
> > google-http-client/src/main/**java/com/google/api/client/**
> > http/javanet/NetHttpResponse.**java:80:
> > return connection.getErrorStream();
> > On 2013/07/23 00:28:08, peleyal wrote:
> >
> >> where can I find it?
> >>
> >
> >
>
http://source.android.com/**source/initializing.html%3Chttp://source.android....>
> >
> >  I added code that won't eat the IOException in the other cases.
> >>
> >
> > did you verify from the implementation if this is the right logic?
> >
> >  On 2013/07/22 12:32:44, yanivi wrote:
> >> > might we be eating the IOException in case of some other networking
> >>
> > problem?
> >
> >> or
> >> > would getErrorStream() then rethrow that same IOException?  I
> >>
> > noticed
> >
> >> > "rememberedException".
> >> >
> >> > By the way, can you please also look at the Android open source
> >>
> > code?
> >
> >
> >
>
https://codereview.appspot.**com/11472043/%3Chttps://codereview.appspot.com/1...>
> >

did you try checking out the sources?
see http://source.android.com/source/initializing.html

[yanivi, 2013-07-25 18:02:35]

On 2013/07/24 20:51:06, yanivi wrote:
> On 2013/07/23 02:02:40, peleyal wrote:
> > The sdk implementation. I don't know where to find the android sources
> > On Jul 22, 2013 10:01 PM, <mailto:yanivi@google.com> wrote:
> > 
> > >
> > > https://codereview.appspot.**com/11472043/diff/14001/**
> > > google-http-client/src/main/**java/com/google/api/client/**
> > >
> >
>
http/javanet/NetHttpResponse.**java<https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java>
> > > File
> > > google-http-client/src/main/**java/com/google/api/client/**
> > > http/javanet/NetHttpResponse.**java
> > > (right):
> > >
> > > https://codereview.appspot.**com/11472043/diff/14001/**
> > > google-http-client/src/main/**java/com/google/api/client/**
> > >
> >
>
http/javanet/NetHttpResponse.**java#newcode80<https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java#newcode80>
> > > google-http-client/src/main/**java/com/google/api/client/**
> > > http/javanet/NetHttpResponse.**java:80:
> > > return connection.getErrorStream();
> > > On 2013/07/23 00:28:08, peleyal wrote:
> > >
> > >> where can I find it?
> > >>
> > >
> > >
> >
>
http://source.android.com/**source/initializing.html%253Chttp://source.androi...>
> > >
> > >  I added code that won't eat the IOException in the other cases.
> > >>
> > >
> > > did you verify from the implementation if this is the right logic?
> > >
> > >  On 2013/07/22 12:32:44, yanivi wrote:
> > >> > might we be eating the IOException in case of some other networking
> > >>
> > > problem?
> > >
> > >> or
> > >> > would getErrorStream() then rethrow that same IOException?  I
> > >>
> > > noticed
> > >
> > >> > "rememberedException".
> > >> >
> > >> > By the way, can you please also look at the Android open source
> > >>
> > > code?
> > >
> > >
> > >
> >
>
https://codereview.appspot.**com/11472043/%253Chttps://codereview.appspot.com...>
> > >
> 
> did you try checking out the sources?
> see http://source.android.com/source/initializing.html

Perhaps Nick can help you with this while you're in Israel?  This is a fairly
serious issue.

[peleyal, 2013-07-29 09:41:24]

https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java
(right):

https://codereview.appspot.com/11472043/diff/14001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java:80:
return connection.getErrorStream();
That's the implementation I found of HttpURLConnectionImpl (hopefully it's the
right file to check...)

  @Override public final InputStream getInputStream() throws IOException {
    if (!doInput) {
      throw new ProtocolException("This protocol does not support input");
    }

    HttpEngine response = getResponse();

    // if the requested file does not exist, throw an exception formerly the
    // Error page from the server was returned if the requested file was
    // text/html this has changed to return FileNotFoundException for all
    // file types
    if (getResponseCode() >= HTTP_BAD_REQUEST) {
      throw new FileNotFoundException(url.toString());
    }

    InputStream result = response.getResponseBody();
    if (result == null) {
      throw new ProtocolException("No response body exists; responseCode=" +
getResponseCode());
    }
    return result;
  }


  /**
   * Returns an input stream from the server in the case of error such as the
   * requested file (txt, htm, html) is not found on the remote server.
   */
  @Override public final InputStream getErrorStream() {
    try {
      HttpEngine response = getResponse();
      if (response.hasResponseBody() && response.getResponseCode() >=
HTTP_BAD_REQUEST) {
        return response.getResponseBody();
      }
      return null;
    } catch (IOException e) {
      return null;
    }
  }

So in my opinion in will work on Android as well (because both of the
implementation return getResponseBody.
 
Anyway I'll have to test it on an Android device (with Nick's help) when I'll be
back in the office. Or, I'll ask Nick to debug \ test it using an Android
device. I've a meeting with him today at 9:30 EST.

I'll update you

On 2013/07/23 02:01:27, yanivi wrote:
> On 2013/07/23 00:28:08, peleyal wrote:
> > where can I find it?
> 
> http://source.android.com/source/initializing.html
> 
> > I added code that won't eat the IOException in the other cases.
> 
> did you verify from the implementation if this is the right logic?
> 
> > On 2013/07/22 12:32:44, yanivi wrote:
> > > might we be eating the IOException in case of some other networking
problem?
> 
> > or
> > > would getErrorStream() then rethrow that same IOException?  I noticed
> > > "rememberedException".
> > > 
> > > By the way, can you please also look at the Android open source code?
> > 
>

[peleyal, 2013-08-02 21:39:36]

ping... :)

[ngmiceli, 2013-08-05 14:02:34]

The change looks good, but not being terribly familiar with the cause of the
leak in the first place I would like to empirically test this again like we did
with the first changeset. Let's also see if we can use adb to run the test as
well.

https://codereview.appspot.com/11472043/diff/25001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java
(right):

https://codereview.appspot.com/11472043/diff/25001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java:80:
// if stream is not null that the error was set on the error stream, otherwise
we should just
If the stream is not null, *then* the error

[peleyal, 2013-08-05 14:55:55]

I tested that code and it worked as expected with the storage sample.
I didn't run this code on Android just because it may take me time to set the
environment and I don't have time for that. That's why I read the documentation
and it looks like it won't affect the android implementation.

[ngmiceli, 2013-08-07 15:39:41]

LGTM

Please fix the relevant findbugs and checkstyle errors before committing.

https://codereview.appspot.com/11472043/diff/40001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java
(right):

https://codereview.appspot.com/11472043/diff/40001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java:56:
public static byte[] INPUT_BUF = new byte[1];
Make final or don't use all-caps. Similarly for ERROR_BUF.

https://codereview.appspot.com/11472043/diff/40001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java:56:
public static byte[] INPUT_BUF = new byte[1];
Make protected with public accessors, or add another suppression for findbugs
(it currently complains about this).

[peleyal, 2013-08-07 19:08:29]

DONE.

https://codereview.appspot.com/11472043/diff/40001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java
(right):

https://codereview.appspot.com/11472043/diff/40001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java:56:
public static byte[] INPUT_BUF = new byte[1];
On 2013/08/07 15:39:42, ngmiceli wrote:
> Make final or don't use all-caps. Similarly for ERROR_BUF.

Done.

[yanivi, 2013-08-07 21:40:20]

https://codereview.appspot.com/11472043/diff/54001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java
(right):

https://codereview.appspot.com/11472043/diff/54001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java:68:
* Upgrade warning: in prior version 1.15 {@link #getContent()} returned
increase the version #s to 1.16 & 1.17 respectively

[peleyal, 2013-08-08 20:51:04]

I already pushed this change so I sent another CL for review (just the version
changes)

Thanks for notice that Yaniv,
Eyal