Closing input stream in HttpResponse.disconnect and fixing NPE

Fix for- 

http://code.google.com/p/google-http-java-client/issues/detail?id=78 : Possible Memory Leak?
and
http://code.google.com/p/google-http-java-client/issues/detail?id=116 : CloseGuard error after EOFException.

After this CL goes in we need to add response.disconnect() to execute() in the service-specific generated libraries.

[yanivi, 2012-05-21 20:39:58]

If it were me, the only change I'd make to HttpResponse would be to add
"ignore()" to the first line of HttpResponse.disconnect().  It's not even
entirely obvious to me that ignore() needs to be called when disconnecting, but
I suppose it shouldn't hurt either.

http://codereview.appspot.com/6225045/diff/1/google-http-client/src/main/java...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
(right):

http://codereview.appspot.com/6225045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:475:
public void close() throws IOException {
why the rename?  I actually prefer "disconnect" over "close" because when I
think "close" I think close the input stream, but "disconnect" implies that
we're also breaking the connection and perhaps releasing some resources.

http://codereview.appspot.com/6225045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:476:
if (response != null) {
response cannot be null

http://codereview.appspot.com/6225045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:477:
final InputStream content = response.getContent();
replace these lines 477-480 and 482 with just "ignore()".

[rmistry, 2012-05-22 12:11:43]

http://codereview.appspot.com/6225045/diff/1/google-http-client/src/main/java...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
(right):

http://codereview.appspot.com/6225045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:475:
public void close() throws IOException {
On 2012/05/21 20:39:58, yanivi wrote:
> why the rename?  I actually prefer "disconnect" over "close" because when I
> think "close" I think close the input stream, but "disconnect" implies that
> we're also breaking the connection and perhaps releasing some resources.

Done.

http://codereview.appspot.com/6225045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:476:
if (response != null) {
On 2012/05/21 20:39:58, yanivi wrote:
> response cannot be null

I was previously doing response = null, but now I decided against it, I do not
think it is needed.

http://codereview.appspot.com/6225045/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:477:
final InputStream content = response.getContent();
On 2012/05/21 20:39:58, yanivi wrote:
> replace these lines 477-480 and 482 with just "ignore()".

Done.

[yanivi, 2012-05-22 13:01:19]

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
(right):

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:43:
* Callers should call {@link #disconnect} when the HTTP response object is no
longer needed. Also,
Good, but please give an example of calling disconnect in a finally block like
the Android docs do.

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:43:
* Callers should call {@link #disconnect} when the HTTP response object is no
longer needed. Also,
We should really test this out with both NetHttpTransport and
ApacheHttpTransport on Android and regular command-line and check that it fixes
the bug and the behavior seems appropriate.  But this may be done after you
submit this changeset.

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:44:
* consider modifying the alive property on {@link HttpTransport} to control
whether the socket
There is no alive property on HttpTransport.  This part of the comment is
specific to NetHttpTransport and belongs there.

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:392:
if (content != null) {
Can't do that because response.getContent() may be null.  just add a boolean
field

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:464:
* input stream
missing period.

also, make it a bit more clear that it didn't close the input stream in prior
versions.

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:471:
response.disconnect();
[aside] Wow, thanks for fixing this NullPointerException on the response!  I
don't know what I was thinking.

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/test/j...
File
google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java
(right):

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/test/j...
google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java:213:
assertTrue(lowLevelHttpResponse.disconnectCalled);
also test that the stream was closed since that's the primary thing we're doing
in this changeset

[rmistry, 2012-05-22 14:37:55]

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
(right):

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:43:
* Callers should call {@link #disconnect} when the HTTP response object is no
longer needed. Also,
On 2012/05/22 13:01:19, yanivi wrote:
> Good, but please give an example of calling disconnect in a finally block like
> the Android docs do.

Done.

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:44:
* consider modifying the alive property on {@link HttpTransport} to control
whether the socket
On 2012/05/22 13:01:19, yanivi wrote:
> There is no alive property on HttpTransport.  This part of the comment is
> specific to NetHttpTransport and belongs there.

Done.

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:392:
if (content != null) {
On 2012/05/22 13:01:19, yanivi wrote:
> Can't do that because response.getContent() may be null.  just add a boolean
> field

Done.

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:464:
* input stream
On 2012/05/22 13:01:19, yanivi wrote:
> missing period.
> 
> also, make it a bit more clear that it didn't close the input stream in prior
> versions.

Done.

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/test/j...
File
google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java
(right):

http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/test/j...
google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java:213:
assertTrue(lowLevelHttpResponse.disconnectCalled);
On 2012/05/22 13:01:19, yanivi wrote:
> also test that the stream was closed since that's the primary thing we're
doing
> in this changeset

Done.

[yanivi, 2012-05-22 15:47:02]

Can you please also update HttpResponseException?  Also, can you please send me
an updated googleplus-simple-cmdline-sample?  I really want to see what this
looks like in practice, and make sure it works as expected.

http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
(right):

http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:52:
response.disconnect();
Ouch!  I see the bug now (I'm pretty sure I saw it reported somewhere, either in
e-mail or in an Issue).  The problem is that if execute() throws an exception we
won't disconnect properly.

Should we add HttpRequest.disconnect() also?

http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:403:
if (contentRead) {
[optional] a bit more elegant to do:

if (!contentRead) {
...
  contentRead = true;
}
return content;

http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/test/j...
File
google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java
(right):

http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/test/j...
google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java:219:
public void testDisconnect() throws IOException {
[optional] would you kindly add a unit test for the case of null
LowLevelHttpResponse.getContent()?

[rmistry, 2012-05-22 16:25:02]

http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
(right):

http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:52:
response.disconnect();
On 2012/05/22 15:47:02, yanivi wrote:
> Ouch!  I see the bug now (I'm pretty sure I saw it reported somewhere, either
in
> e-mail or in an Issue).  The problem is that if execute() throws an exception
we
> won't disconnect properly.
> 
> Should we add HttpRequest.disconnect() also?

Let me know what you think of what I did in HttpRequest.java

http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:403:
if (contentRead) {
On 2012/05/22 15:47:02, yanivi wrote:
> [optional] a bit more elegant to do:
> 
> if (!contentRead) {
> ...
>   contentRead = true;
> }
> return content;

Done.

http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/test/j...
File
google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java
(right):

http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/test/j...
google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java:219:
public void testDisconnect() throws IOException {
On 2012/05/22 15:47:02, yanivi wrote:
> [optional] would you kindly add a unit test for the case of null
> LowLevelHttpResponse.getContent()?

Done.

[rmistry, 2012-05-22 16:29:29]

On 2012/05/22 15:47:02, yanivi wrote:
> Can you please also update HttpResponseException?  Also, can you please send
me
> an updated googleplus-simple-cmdline-sample?  I really want to see what this
> looks like in practice, and make sure it works as expected.
> 

Not sure what you wanted me to send you in googleplus-simple-cmdline-sample. I
think all we need to do is change pom.xml to 1.10.0-beta-SNAPSHOT and add

HttpResponse response = request.execute();
try {
  parseResponse(response);
} finally {
  response.disconnect();
}

Let me know if I misunderstood what you were asking.

>
http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/main/j...
> File
> google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
> (right):
> 
>
http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/main/j...
>
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:52:
> response.disconnect();
> Ouch!  I see the bug now (I'm pretty sure I saw it reported somewhere, either
in
> e-mail or in an Issue).  The problem is that if execute() throws an exception
we
> won't disconnect properly.
> 
> Should we add HttpRequest.disconnect() also?
> 
>
http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/main/j...
>
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:403:
> if (contentRead) {
> [optional] a bit more elegant to do:
> 
> if (!contentRead) {
> ...
>   contentRead = true;
> }
> return content;
> 
>
http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/test/j...
> File
>
google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java
> (right):
> 
>
http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/test/j...
>
google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java:219:
> public void testDisconnect() throws IOException {
> [optional] would you kindly add a unit test for the case of null
> LowLevelHttpResponse.getContent()?

[rmistry, 2012-05-23 11:18:25]

Ran 'hg pull -u'

[yanivi, 2012-05-23 13:23:42]

http://codereview.appspot.com/6225045/diff/15001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/6225045/diff/15001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:721:
public HttpResponse execute() throws IOException {
JavaDoc should clearly document the disconnect error-handling behavior, and what
we expect users to do (if anything)

http://codereview.appspot.com/6225045/diff/15001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:721:
public HttpResponse execute() throws IOException {
[optional]  There is an alternative design you may want to consider.  I don't
feel knowledgeable enough to determine which is better.

The alternative design is to deprecate the HttpResponse.disconnect() method and
instead move it to HttpRequest.disconnect().  Users already have to use a
try-finally block and call disconnect(), so we might as well piggyback on it. 
That way, we won't need to do anything inside request.execute(), and in fact we
give the developer an option to only disconnect after a sequence of requests is
completed, rather than disconnect after every request.

http://codereview.appspot.com/6225045/diff/15001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:896:
lowLevelHttpRequest.disconnect();
Definitely *not* a good idea to disconnect for the success use case before the
stream has been read :)

I updated the googleplus-simple-cmdline-sample to use the newly built http
library, and sure enough:


java.io.EOFException
	at java.util.zip.GZIPInputStream.readUByte(GZIPInputStream.java:224)
	at java.util.zip.GZIPInputStream.readUShort(GZIPInputStream.java:215)
	at java.util.zip.GZIPInputStream.readHeader(GZIPInputStream.java:153)
	at java.util.zip.GZIPInputStream.<init>(GZIPInputStream.java:75)
	at java.util.zip.GZIPInputStream.<init>(GZIPInputStream.java:85)
	at com.google.api.client.http.HttpResponse.getContent(HttpResponse.java:410)
	at
com.google.api.client.http.json.JsonHttpParser.parserForResponse(JsonHttpParser.java:115)
	at com.google.api.client.http.json.JsonHttpParser.parse(JsonHttpParser.java:87)
	at com.google.api.client.http.HttpResponse.parseAs(HttpResponse.java:509)
	at
com.google.api.services.samples.googleplus.cmdline.simple.GooglePlusSample.parseResponse(GooglePlusSample.java:155)
	at
com.google.api.services.samples.googleplus.cmdline.simple.GooglePlusSample.run(GooglePlusSample.java:185)
	at
com.google.api.services.samples.googleplus.cmdline.simple.GooglePlusSample.main(GooglePlusSample.java:195)

This is my point regarding updating the sample: you will catch a lot of
implementation/design problems that way.

http://codereview.appspot.com/6225045/diff/15001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:896:
lowLevelHttpRequest.disconnect();
There is another use case we need to disconnect on: the case of a throwable
being thrown.  For example, EOFException, or even a bug in the code that causes
a NullPointerException, or even an error like OutOfMemoryError.  That's why the
Android recommendation is to disconnect in a finally block.  We may want to do
the same.

[rmistry, 2012-05-23 17:13:30]

http://codereview.appspot.com/6225045/diff/15001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/6225045/diff/15001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:721:
public HttpResponse execute() throws IOException {
On 2012/05/23 13:23:43, yanivi wrote:
> [optional]  There is an alternative design you may want to consider.  I don't
> feel knowledgeable enough to determine which is better.
> 
> The alternative design is to deprecate the HttpResponse.disconnect() method
and
> instead move it to HttpRequest.disconnect().  Users already have to use a
> try-finally block and call disconnect(), so we might as well piggyback on it. 
> That way, we won't need to do anything inside request.execute(), and in fact
we
> give the developer an option to only disconnect after a sequence of requests
is
> completed, rather than disconnect after every request.

How about instead HttpResponse.disconnect calling HttpRequest.disconnect, I say
this because having updated internal references recently users seem to always
pass HttpResponse around rather than HttpRequest. HttpRequest is normally just
used for execute and discarded.

Also, just to clarify, I am still not completely convinced what the right
approach is here. I wanted to try a few things and test it out and see what you
think. So please comment on alternative design suggestions if you have any or if
you like what you see so far.

http://codereview.appspot.com/6225045/diff/15001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:721:
public HttpResponse execute() throws IOException {
On 2012/05/23 13:23:43, yanivi wrote:
> JavaDoc should clearly document the disconnect error-handling behavior, and
what
> we expect users to do (if anything)

Done.

http://codereview.appspot.com/6225045/diff/15001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:896:
lowLevelHttpRequest.disconnect();
On 2012/05/23 13:23:43, yanivi wrote:
> Definitely *not* a good idea to disconnect for the success use case before the
> stream has been read :)
> 
> I updated the googleplus-simple-cmdline-sample to use the newly built http
> library, and sure enough:
> 
> 
> java.io.EOFException
> 	at java.util.zip.GZIPInputStream.readUByte(GZIPInputStream.java:224)
> 	at java.util.zip.GZIPInputStream.readUShort(GZIPInputStream.java:215)
> 	at java.util.zip.GZIPInputStream.readHeader(GZIPInputStream.java:153)
> 	at java.util.zip.GZIPInputStream.<init>(GZIPInputStream.java:75)
> 	at java.util.zip.GZIPInputStream.<init>(GZIPInputStream.java:85)
> 	at com.google.api.client.http.HttpResponse.getContent(HttpResponse.java:410)
> 	at
>
com.google.api.client.http.json.JsonHttpParser.parserForResponse(JsonHttpParser.java:115)
> 	at
com.google.api.client.http.json.JsonHttpParser.parse(JsonHttpParser.java:87)
> 	at com.google.api.client.http.HttpResponse.parseAs(HttpResponse.java:509)
> 	at
>
com.google.api.services.samples.googleplus.cmdline.simple.GooglePlusSample.parseResponse(GooglePlusSample.java:155)
> 	at
>
com.google.api.services.samples.googleplus.cmdline.simple.GooglePlusSample.run(GooglePlusSample.java:185)
> 	at
>
com.google.api.services.samples.googleplus.cmdline.simple.GooglePlusSample.main(GooglePlusSample.java:195)
> 
> This is my point regarding updating the sample: you will catch a lot of
> implementation/design problems that way.

Yes true. I ran it through the sample as well this time.

http://codereview.appspot.com/6225045/diff/15001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:896:
lowLevelHttpRequest.disconnect();
On 2012/05/23 13:23:43, yanivi wrote:
> There is another use case we need to disconnect on: the case of a throwable
> being thrown.  For example, EOFException, or even a bug in the code that
causes
> a NullPointerException, or even an error like OutOfMemoryError.  That's why
the
> Android recommendation is to disconnect in a finally block.  We may want to do
> the same.

In this case I have to add a disconnect to HttpRequest. I was hoping to avoid
doing this hoping that HttpRequest along with HttpResponse will be able to
disconnect appropriately by themselves. But I see your point for NPEs and OOMs
we should allow users to disconnect.

[rmistry, 2012-05-29 13:24:19]

Updated CL based on recent conversation in the internal bug.

[yanivi, 2012-05-29 23:03:39]

Some very tricky issues with this changeset.  The good news is that I think I
finally have a clearer understanding of what we are ideally looking for.

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
(right):

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:46:
* Callers should call {@link #disconnect} when the HTTP response object is no
longer needed.
Should we specify that disconnect doesn't need to be called if the response
stream is properly closed?  this is apparently the case for HttpURLConnection.

Here's a case in point: what do we do about
JsonHttpRequest.executeAsInputStream()?  that method returns only the
InputStream, not the HttpResponse.  But in practice this should be okay.

Actually, please look carefully at all callers of
JsonHttpRequest.executeUnparsed (and all callers of those transitively).  For
example, the JsonHttpRequest.download() method needs to call disconnect in a
finally block.

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:52:
HttpResponse response = request.execute();
For this to be provably correct, HttpRequest.execute() needs to be rock-solid. 
The only way to make it correct is to put a try-finally block inside
HttpRequest.execute() to call disconnect() unless successful, much like the code
you wrote in NetHttpRequest.

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponseException.java
(right):

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpResponseException.java:62:
try {
No for the new lines of code here.  Instead, I think it should be the
responsibility of the callers of these constructors to disconnect the response. 
We should document this in the JavaDoc.

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java
(right):

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java:54:
public LowLevelHttpResponse execute() throws IOException {
JavaDoc of LowLevelHttpRequest.execute() should specify that disconnect must be
called if it throws something

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java:92:
return new NetHttpResponse(connection);
this line could throw an exception too, so better to split it up, e.g.
NetHttpResponse response = ...
successfulConnection = true;
return response;

[rmistry, 2012-05-30 13:07:25]

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
(right):

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:46:
* Callers should call {@link #disconnect} when the HTTP response object is no
longer needed.
On 2012/05/29 23:03:39, yanivi wrote:
> Should we specify that disconnect doesn't need to be called if the response
> stream is properly closed?  this is apparently the case for HttpURLConnection.
> 

Done.

> Here's a case in point: what do we do about
> JsonHttpRequest.executeAsInputStream()?  that method returns only the
> InputStream, not the HttpResponse.  But in practice this should be okay.
> 

Yes this should be okay because we have a comment says "Callers are responsible
for closing the input stream." and as discussed yesterday closing the stream
seems to be enough to get rid of any leaks.

> Actually, please look carefully at all callers of
> JsonHttpRequest.executeUnparsed (and all callers of those transitively).  For
> example, the JsonHttpRequest.download() method needs to call disconnect in a
> finally block.

JsonHttpRequest.download does not really need to call disconnect because
HttpResponse#download closes the response content. But I still added it to be
consistent.

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:52:
HttpResponse response = request.execute();
On 2012/05/29 23:03:39, yanivi wrote:
> For this to be provably correct, HttpRequest.execute() needs to be rock-solid.

> The only way to make it correct is to put a try-finally block inside
> HttpRequest.execute() to call disconnect() unless successful, much like the
code
> you wrote in NetHttpRequest.

I think HttpRequest.execute() looks like its already doing the right thing. If
response is unsuccessful we throw HttpResponseException(response) which parses
and closes the stream. If we are retrying we first call response.ignore if there
is an IOException then we rethrow it.
Please let me know if I am wrong.

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponseException.java
(right):

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpResponseException.java:62:
try {
On 2012/05/29 23:03:39, yanivi wrote:
> No for the new lines of code here.  Instead, I think it should be the
> responsibility of the callers of these constructors to disconnect the
response. 
> We should document this in the JavaDoc.

Done. Though is this even needed? since we have concluded that parsing and
closing the content stream is sufficient.

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java
(right):

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java:54:
public LowLevelHttpResponse execute() throws IOException {
On 2012/05/29 23:03:39, yanivi wrote:
> JavaDoc of LowLevelHttpRequest.execute() should specify that disconnect must
be
> called if it throws something

Sorry do not understand this comment, JavaDoc of LowLevelHttpRequest.execute()
should specify which disconnect must be called?

http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java:92:
return new NetHttpResponse(connection);
On 2012/05/29 23:03:39, yanivi wrote:
> this line could throw an exception too, so better to split it up, e.g.
> NetHttpResponse response = ...
> successfulConnection = true;
> return response;

Done.

[yanivi, 2012-05-30 14:19:07]

http://codereview.appspot.com/6225045/diff/30010/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/6225045/diff/30010/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:863:
errorHandled = unsuccessfulResponseHandler.handleResponse(this, response,
retrySupported);
So here's what I think you're missing in this discussion:

Suppose handleResponse throws an exception.  Maybe it's an IOException or a
NullPointerException or an OutOfMemoryError.  In that case, we would leak some
resources because the socket won't close.  More generally, we should be paranoid
and assume any line of code could cause an exception to be thrown.  Be paranoid.

So far we've only been thinking about two cases: success case and error case,
both of which involve an HTTP response but with differing status codes.  But I'm
talking about a third use case of unexpected exceptions thrown at the most
inconvenient time.

Now, please re-read my comments from my last review and view them in that
context.  I'm not asking to call disconnect in a finally block to be
"consistent".  I'm asking to do it if and only if it is the correct
error-handling code.  Now is the best time to take a rigorous approach to this
problem.  It's easier to solve it now than to have to revisit this issue every
time someone reports yet another error case where we are not properly freeing
resources.

So here's what we should do: methods that receive an HttpResponse or InputStream
must either:

1. Methods that return an HttpResponse or InputStream should have JavaDoc that
clearly indicates that callers must disconnect in a try-finally block. 
Currently we say "callers are responsible to close the stream" in some places,
but I think that wording is not strong enough, because we need to say that it
should be done in a finally block.

2. Methods that receive an HttpResponse or InputStream must must have robust
error-handling logic that disconnects in a finally block in case an unexpected
exception is thrown.  Even if they fall in the category of #1, any code that
runs between the time the response is received in the method until the return
statement should be wrapped in a finally block.

Does that make sense?

http://codereview.appspot.com/6225045/diff/30010/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java
(right):

http://codereview.appspot.com/6225045/diff/30010/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java:213:
} finally {
ok now that I've looked at this more carefully, it looks like this code is
unnecessary.  please remove it.

[rmistry, 2012-05-30 16:10:17]

http://codereview.appspot.com/6225045/diff/30010/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/6225045/diff/30010/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:863:
errorHandled = unsuccessfulResponseHandler.handleResponse(this, response,
retrySupported);
On 2012/05/30 14:19:07, yanivi wrote:
> So here's what I think you're missing in this discussion:
> 
> Suppose handleResponse throws an exception.  Maybe it's an IOException or a
> NullPointerException or an OutOfMemoryError.  In that case, we would leak some
> resources because the socket won't close.  More generally, we should be
paranoid
> and assume any line of code could cause an exception to be thrown.  Be
paranoid.
> 
> So far we've only been thinking about two cases: success case and error case,
> both of which involve an HTTP response but with differing status codes.  But
I'm
> talking about a third use case of unexpected exceptions thrown at the most
> inconvenient time.
> 
> Now, please re-read my comments from my last review and view them in that
> context.  I'm not asking to call disconnect in a finally block to be
> "consistent".  I'm asking to do it if and only if it is the correct
> error-handling code.  Now is the best time to take a rigorous approach to this
> problem.  It's easier to solve it now than to have to revisit this issue every
> time someone reports yet another error case where we are not properly freeing
> resources.
> 
> So here's what we should do: methods that receive an HttpResponse or
InputStream
> must either:
> 
> 1. Methods that return an HttpResponse or InputStream should have JavaDoc that
> clearly indicates that callers must disconnect in a try-finally block. 
> Currently we say "callers are responsible to close the stream" in some places,
> but I think that wording is not strong enough, because we need to say that it
> should be done in a finally block.
> 
> 2. Methods that receive an HttpResponse or InputStream must must have robust
> error-handling logic that disconnects in a finally block in case an unexpected
> exception is thrown.  Even if they fall in the category of #1, any code that
> runs between the time the response is received in the method until the return
> statement should be wrapped in a finally block.
> 
> Does that make sense?

Yes, Done (I think).

http://codereview.appspot.com/6225045/diff/30010/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java
(right):

http://codereview.appspot.com/6225045/diff/30010/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java:213:
} finally {
On 2012/05/30 14:19:07, yanivi wrote:
> ok now that I've looked at this more carefully, it looks like this code is
> unnecessary.  please remove it.

We will probably still need to disconnect the response if an exception is
encountered in response.download(outputStream).

[yanivi, 2012-05-30 21:07:26]

almost there...

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:721:
public HttpResponse execute() throws IOException {
would be nice to add JavaDoc about disconnecting in  a finally

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:845:
response = new HttpResponse(this, lowLevelHttpRequest.execute());
what happens if an exception is thrown inside constructor new HttpResponse()? do
we need to call getContent().close() and LowLevelHttpResponse.disconnect() ?

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
(right):

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:336:
public InputStream getContent() throws IOException {
would help to add JavaDoc explaining that the content must be closed in a
finally, and also that you don't need to disconnect if you close the content

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:336:
public InputStream getContent() throws IOException {
implementation of getContent has a similar problem as HttpRequest.execute().  If
any of the code inside throws an exception of some kind we won't actually close
the stream.  so perhaps we need a try-finally and call
lowLevelResponseContent.close() or response.disconnect()

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpTransport.java
(right):

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpTransport.java:25:
* Users should consider modifying the alive property on {@link NetHttpTransport}
to control
may be better to point the user at this documentation:

http://docs.oracle.com/javase/7/docs/technotes/guides/net/http-keepalive.html

I think otherwise, this comment taken out of context is difficult to understand.

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java
(right):

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:422:
* Callers are responsible for closing the input stream.
expand on this comment and give sample code

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:438:
inputStream = response.getContent();
I think response.getContent() should instead do its own try-finally

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java
(right):

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java:201:
inputStream = response.getContent();
again HttpResponse.getContent() should do its own try-finally

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java:227:
response.download(outputStream);
download already does its own try-finally

[rmistry, 2012-05-31 12:25:38]

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:721:
public HttpResponse execute() throws IOException {
On 2012/05/30 21:07:26, yanivi wrote:
> would be nice to add JavaDoc about disconnecting in  a finally

Done.

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:845:
response = new HttpResponse(this, lowLevelHttpRequest.execute());
On 2012/05/30 21:07:26, yanivi wrote:
> what happens if an exception is thrown inside constructor new HttpResponse()?
do
> we need to call getContent().close() and LowLevelHttpResponse.disconnect() ?

Done.

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
(right):

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:336:
public InputStream getContent() throws IOException {
On 2012/05/30 21:07:26, yanivi wrote:
> would help to add JavaDoc explaining that the content must be closed in a
> finally, and also that you don't need to disconnect if you close the content

Done.

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:336:
public InputStream getContent() throws IOException {
On 2012/05/30 21:07:26, yanivi wrote:
> implementation of getContent has a similar problem as HttpRequest.execute(). 
If
> any of the code inside throws an exception of some kind we won't actually
close
> the stream.  so perhaps we need a try-finally and call
> lowLevelResponseContent.close() or response.disconnect()

Done.

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpTransport.java
(right):

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpTransport.java:25:
* Users should consider modifying the alive property on {@link NetHttpTransport}
to control
On 2012/05/30 21:07:26, yanivi wrote:
> may be better to point the user at this documentation:
> 
> http://docs.oracle.com/javase/7/docs/technotes/guides/net/http-keepalive.html
> 
> I think otherwise, this comment taken out of context is difficult to
understand.

Done.

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java
(right):

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:422:
* Callers are responsible for closing the input stream.
On 2012/05/30 21:07:26, yanivi wrote:
> expand on this comment and give sample code

Done.

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java:438:
inputStream = response.getContent();
On 2012/05/30 21:07:26, yanivi wrote:
> I think response.getContent() should instead do its own try-finally

Yes, it is now. Done.

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java
(right):

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java:201:
inputStream = response.getContent();
On 2012/05/30 21:07:26, yanivi wrote:
> again HttpResponse.getContent() should do its own try-finally

Done.

http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java:227:
response.download(outputStream);
On 2012/05/30 21:07:26, yanivi wrote:
> download already does its own try-finally

Done.

[yanivi, 2012-05-31 14:59:18]

LGTM

http://codereview.appspot.com/6225045/diff/29002/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/6225045/diff/29002/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:717:
* response stream is properly closed. Example usage:
just to be nice: use </p>

[rmistry, 2012-05-31 15:07:26]

http://codereview.appspot.com/6225045/diff/29002/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/6225045/diff/29002/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:717:
* response stream is properly closed. Example usage:
On 2012/05/31 14:59:18, yanivi wrote:
> just to be nice: use </p>

Done.