Response.close() nullability issue. #3383

Activity

1. swankjesse commented on Jun 2, 2017

   

   swankjesse

   on Jun 2, 2017

   Collaborator

   More actions

   We could throw an IllegalStateException instead. Applications shouldn't ever attempt to close the response returned from a cacheResponse() or networkResponse().
2. Tolriq commented on Jun 2, 2017

   

   Tolriq

   on Jun 2, 2017

   ContributorAuthor

   More actions

   Not sure to understand

   From code https://github.com/square/okhttp/blob/c70e35802e984e8b9bf3fbf1479deb37c0bdfa43/okhttp/src/main/java/okhttp3/Response.java#L274

   Response.close() correctly only deals with body (ResponseBody) and does not touch the others.

   Finally maybe I'm wrong but in the end just adding a body != null check sound like the proper handling as there's no real error to manage in fact, a Closeable should not NPE on close.
3. swankjesse commented on Jun 2, 2017

   

   swankjesse

   on Jun 2, 2017

   Collaborator

   More actions

   It's an error to attempt to close a response that doesn't have a body.
4. swankjesse

   added a commit that references this issue on Jun 10, 2017

   Clarify documentation on closing WebSockets and Responses.

   ...

   c133700
5. swankjesse

   closed this as completedin #3398on Jun 10, 2017
6. gilday commented on Nov 14, 2018

   

   gilday

   on Nov 14, 2018

   More actions

   It's an error to attempt to close a response that doesn't have a body.

   @swankjesse how strongly do you hold this opinion? If this were not an error condition, then users could use Response in a try-with-resources statement in scenarios where the body is not considered. Take this example which checks for a successful status code and ignores the body

   Response response = null;
   try {
     response = client.newCall(request).execute();
     if (!response.isSuccessful()) {
       throw new IllegalStateException("HTTP server returned error code " + response.code());
     }
   } finally {
     if (response != null && response.body() != null) {
       response.close();
     }
   }

   If this code didn't need to first check response.body != null to avoid an error condition, then we could get rid of boilerplate with a try-with-resources statement.

   try(Response response = client.newCall(request).execute()) {
     if (!response.isSuccessful()) {
       throw new IllegalStateException("HTTP server returned error code " + response.code());
     }
   }
7. swankjesse commented on Nov 15, 2018

   

   swankjesse

   on Nov 15, 2018

   Collaborator

   More actions

   Your example above is safe. You have to try hard to get a response without a body – it’s the response used by Response.networkResponse() or Response.cacheResponse().

   👍React with 👍1Reacted by Johnathan Gilday
8. gilday commented on Nov 15, 2018

   

   gilday

   on Nov 15, 2018

   More actions

   Cool, that helps. I was running into trouble in a unit test where I was building a Response myself and I omitted the body e.g.

       final Response response =
           new Response.Builder()
               .request(success.request())
               .protocol(Protocol.HTTP_1_1)
               .message("OK")
               .code(204)
               .build();

   but I can fix this by adding an empty body to the builder chain and remain confident that for my use case responses should always have a non-null body.

   .body(EMPTY_RESPONSE)

   thanks!