Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(178)

Issue 6225045: Closing input stream in HttpResponse.disconnect and fixing NPE (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 2 months ago by rmistry
Modified:
9 years, 2 months ago
Reviewers:
yanivi
CC:
mlinder
Base URL:
https://google-http-java-client.googlecode.com/hg/
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 6

Patch Set 2 : Addressing code review comments #

Total comments: 12

Patch Set 3 : Addressing code review comments #

Total comments: 6

Patch Set 4 : Addressing code review comments #

Patch Set 5 : Minor fix #

Patch Set 6 : hg pull -u #

Patch Set 7 : Addressing code review comments #

Patch Set 8 : Minor fix #

Total comments: 8

Patch Set 9 : Addressing code review comments #

Patch Set 10 : Minor fix #

Patch Set 11 : Minor fix #

Patch Set 12 : Updating CL with recent conversation #

Patch Set 13 : Minor fix #

Total comments: 10

Patch Set 14 : Addressing code review comments #

Patch Set 15 : Minor fix #

Patch Set 16 : Minor fix #

Total comments: 4

Patch Set 17 : Addressing code review comments #

Total comments: 18

Patch Set 18 : Addressing code review comments #

Patch Set 19 : Minor fix #

Total comments: 2

Patch Set 20 : Addressing code review comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+333 lines, -85 lines) Patch
M google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 4 chunks +79 lines, -37 lines 0 comments Download
M google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 5 chunks +66 lines, -19 lines 0 comments Download
M google-http-client/src/main/java/com/google/api/client/http/HttpResponseException.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +27 lines, -17 lines 0 comments Download
M google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +11 lines, -2 lines 0 comments Download
M google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpTransport.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +7 lines, -0 lines 0 comments Download
google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpClient.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +32 lines, -5 lines 0 comments Download
M google-http-client/src/main/java/com/google/api/client/http/json/JsonHttpRequest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +21 lines, -3 lines 0 comments Download
M google-http-client/src/test/java/com/google/api/client/http/HttpResponseExceptionTest.java View 1 2 3 4 5 6 7 8 2 chunks +0 lines, -2 lines 0 comments Download
M google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java View 1 2 3 4 5 7 2 chunks +90 lines, -0 lines 0 comments Download

Messages

Total messages: 20
rmistry
9 years, 2 months ago (2012-05-21 18:09:48 UTC) #1
yanivi
If it were me, the only change I'd make to HttpResponse would be to add ...
9 years, 2 months ago (2012-05-21 20:39:58 UTC) #2
rmistry
http://codereview.appspot.com/6225045/diff/1/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.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/com/google/api/client/http/HttpResponse.java#newcode475 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, ...
9 years, 2 months ago (2012-05-22 12:11:43 UTC) #3
yanivi
http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java 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/java/com/google/api/client/http/HttpResponse.java#newcode43 google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:43: * Callers should call {@link #disconnect} when the HTTP ...
9 years, 2 months ago (2012-05-22 13:01:19 UTC) #4
rmistry
http://codereview.appspot.com/6225045/diff/2002/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java 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/java/com/google/api/client/http/HttpResponse.java#newcode43 google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:43: * Callers should call {@link #disconnect} when the HTTP ...
9 years, 2 months ago (2012-05-22 14:37:55 UTC) #5
yanivi
Can you please also update HttpResponseException? Also, can you please send me an updated googleplus-simple-cmdline-sample? ...
9 years, 2 months ago (2012-05-22 15:47:02 UTC) #6
rmistry
http://codereview.appspot.com/6225045/diff/7001/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java 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/java/com/google/api/client/http/HttpResponse.java#newcode52 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 ...
9 years, 2 months ago (2012-05-22 16:25:02 UTC) #7
rmistry
On 2012/05/22 15:47:02, yanivi wrote: > Can you please also update HttpResponseException? Also, can you ...
9 years, 2 months ago (2012-05-22 16:29:29 UTC) #8
rmistry
Ran 'hg pull -u'
9 years, 2 months ago (2012-05-23 11:18:25 UTC) #9
yanivi
http://codereview.appspot.com/6225045/diff/15001/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java 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/java/com/google/api/client/http/HttpRequest.java#newcode721 google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:721: public HttpResponse execute() throws IOException { JavaDoc should clearly ...
9 years, 2 months ago (2012-05-23 13:23:42 UTC) #10
rmistry
http://codereview.appspot.com/6225045/diff/15001/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java 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/java/com/google/api/client/http/HttpRequest.java#newcode721 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, ...
9 years, 2 months ago (2012-05-23 17:13:30 UTC) #11
rmistry
Updated CL based on recent conversation in the internal bug.
9 years, 2 months ago (2012-05-29 13:24:19 UTC) #12
yanivi
Some very tricky issues with this changeset. The good news is that I think I ...
9 years, 2 months ago (2012-05-29 23:03:39 UTC) #13
rmistry
http://codereview.appspot.com/6225045/diff/21001/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java 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/java/com/google/api/client/http/HttpResponse.java#newcode46 google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:46: * Callers should call {@link #disconnect} when the HTTP ...
9 years, 2 months ago (2012-05-30 13:07:25 UTC) #14
yanivi
http://codereview.appspot.com/6225045/diff/30010/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java 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/java/com/google/api/client/http/HttpRequest.java#newcode863 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 ...
9 years, 2 months ago (2012-05-30 14:19:07 UTC) #15
rmistry
http://codereview.appspot.com/6225045/diff/30010/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java 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/java/com/google/api/client/http/HttpRequest.java#newcode863 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 ...
9 years, 2 months ago (2012-05-30 16:10:17 UTC) #16
yanivi
almost there... http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java 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/java/com/google/api/client/http/HttpRequest.java#newcode721 google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:721: public HttpResponse execute() throws IOException { would ...
9 years, 2 months ago (2012-05-30 21:07:26 UTC) #17
rmistry
http://codereview.appspot.com/6225045/diff/22009/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java 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/java/com/google/api/client/http/HttpRequest.java#newcode721 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, ...
9 years, 2 months ago (2012-05-31 12:25:38 UTC) #18
yanivi
LGTM http://codereview.appspot.com/6225045/diff/29002/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java 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/java/com/google/api/client/http/HttpRequest.java#newcode717 google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:717: * response stream is properly closed. Example usage: ...
9 years, 2 months ago (2012-05-31 14:59:18 UTC) #19
rmistry
9 years, 2 months ago (2012-05-31 15:07:26 UTC) #20
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b