Add Response.isClosed()

[NicoNes]

This PR is the follow-up to the issue and discussion we had with @ronsigal.

To sum up it was about adding a method in the Response class to know when a response instance is closed. So that we no longer need to write code like:

try {
   if (response.getEntity() != null) return response;
}
catch(IllegalStateException ise) {
   // IllegalStateException from ClientResponse.getEntity() means the response is closed and got no entity
} 

but instead:

if (!response.isClosed() && response.getEntity() != null) {
   return response;
}

I agree that the default implementation I proposed is a bit weird, not intuitive etc . I did it so that vendors don't need to make changes on their side.
I'm OK to remove it if you think it's not necessary.

Thanks

[andymc12]

LGTM - thanks!

[mkarg]

LGTM. Thank you for this contribution!

[mkarg]

I'd like to propose that we add this to 3.1 already as it does not change the API in an incompatible way thanks to the default implementation.

Speaking of the default implementation I'd like to kindly ask all vendors to override it using an implementation that works with higher performance than the default solution.

Last but not least I am kindly asking for TCK coverage.

[NicoNes]

@mkarg do you want me to add somehing as follow in the doc ?

For backwards compatibility, there is a default implementation that just relies on IllegalStateException thrown by hasEntity when response instance is closed.
Implementations should consider replacing this with a more performant implementation

[mkarg]

@mkarg do you want me to add somehing as follow in the doc ?

For backwards compatibility, there is a default implementation that just relies on IllegalStateException thrown by hasEntity when response instance is closed.
Implementations should consider replacing this with a more performant implementation

I do not see a need for this, actually.

[mkarg]

@NicoNes If you like to contribute it: For this feature we need a note in the "Changes since" section of the spec, an example in the examples folder would be great, and the TCK needs to test that the actual implementation provided by the vendor works just as correct as your default implementation.

[spericas]

@NicoNes If you like to contribute it: For this feature we need a note in the "Changes since" section of the spec

That's required only for changes to the spec document.

an example in the examples folder would be great, and the TCK needs to test that the actual implementation provided by the vendor works just as correct as your default implementation.

Given the simplicity of this (just a convenience method), more of a nice to have IMO.

[NicoNes]

Well @spericas and @mkarg given your two different opinions I don't really know what I have to do. Please let me know.

Thanks

[mkarg]

@NicoNes If you like to contribute it: For this feature we need a note in the "Changes since" section of the spec

That's required only for changes to the spec document.

an example in the examples folder would be great, and the TCK needs to test that the actual implementation provided by the vendor works just as correct as your default implementation.

Given the simplicity of this (just a convenience method), more of a nice to have IMO.

Okay, let's rephrase: It would be nice to have it, so users know about its existence and usage. :-)

[spericas]

@NicoNes The PR has all the necessary approvals to be merged.