Add documentation to clarify if ResponseBody.close() is necessary #3400
Conversation
tejas-n
force-pushed
the
responsebody-close-docs
branch
from
c2500ca
to
3e6572f
Compare
| /** | ||
| * The deserialized response body of a {@linkplain #isSuccessful() successful} response. | ||
| * | ||
| * <p>If the body is of type {@link ResponseBody}, calling {@link ResponseBody#close close} is |
There was a problem hiding this comment.
I don't think this is helpful because it splits the type system.
By following this doc you need to differentiate between responses that are created from streaming endpoints and responses that are not. If you write general purpose code to accept a Response, you need to clarify whether it's a streaming one. Also if you add @Streaming you create a resource leak.
It's simpler and more resilient to document that this must be closed under all circumstances.
There was a problem hiding this comment.
Thanks for the reply. Yeah, that makes sense. So you are suggesting "If the body is of type ResponseBody, close() should always be called to avoid leaking resources"? Am I correct?
There was a problem hiding this comment.
@swankjesse I've updated the PR, can you let me know if that looks correct, please?
tejas-n
force-pushed
the
responsebody-close-docs
branch
from
3e6572f
to
5dd32be
Compare
| * <p>If the body is of type {@link ResponseBody}, {@link ResponseBody#close()} must always be | ||
| * called to avoid leaking resources |
There was a problem hiding this comment.
This is only true of @Streaming endpoints. I think this doc needs to live only on the @Streaming annotation and not here.
The one down below is still useful, however.
There was a problem hiding this comment.
Thanks, that makes sense. I'll try to update it tonight.
Closes #2950, closes #2929