You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by GitBox <gi...@apache.org> on 2021/09/17 02:35:17 UTC
[GitHub] [cxf] reta opened a new pull request #851: CXF-8597: CXF JAXRS client not closing HTTP connections
reta opened a new pull request #851:
URL: https://github.com/apache/cxf/pull/851
Relates to https://github.com/apache/cxf/pull/697/files, the entity input stream is indeed not closed after consumption. But according to the specification for `public abstract <T> T readEntity(Class<T> entityType, Annotation[] annotations)`:
> A message instance returned from this method will be cached for subsequent retrievals via getEntity(). Unless the supplied entitytype is an input stream, this method automatically closes the an unconsumed original response entity data stream if open.
TODO:
- [ ] Add Test Case(s)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #851: CXF-8597: CXF JAXRS client not closing HTTP connections
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #851:
URL: https://github.com/apache/cxf/pull/851#discussion_r711161589
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -418,7 +418,7 @@ private Link makeAbsoluteLink(Link link) {
public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns)
throws ProcessingException, IllegalStateException {
- return doReadEntity(cls, t, anns, false);
+ return doReadEntity(cls, t, anns, !InputStream.class.isAssignableFrom(cls));
Review comment:
Thanks @andymc12 , yes, I was thinking about `ClientProxyImpl.handleResponse` first to basically use try-with-resources on response, I will explore this path shortly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] andymc12 commented on a change in pull request #851: CXF-8597: CXF JAXRS client not closing HTTP connections
Posted by GitBox <gi...@apache.org>.
andymc12 commented on a change in pull request #851:
URL: https://github.com/apache/cxf/pull/851#discussion_r711149701
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -418,7 +418,7 @@ private Link makeAbsoluteLink(Link link) {
public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns)
throws ProcessingException, IllegalStateException {
- return doReadEntity(cls, t, anns, false);
+ return doReadEntity(cls, t, anns, !InputStream.class.isAssignableFrom(cls));
Review comment:
In the case of proxy clients, I think CXF should be closing the `Response`. I think we may already do that in some cases where the `Response` object is not visible to the user. Maybe we should modify the `WebClient.castResponse` method to also conditionally close the response - something like this:
```
@SuppressWarnings("unchecked")
private <T> T castResponse(Response r, Class<T> responseClass) {
if (responseClass == Response.class)
return r;
try (Response r2 = r) {
return (T) r.getEntity();
}
}
```
This might need some testing, but I cannot think of a case where we would need to keep the `Response` open if the user cannot access it. An alternative would be to close the `Response` in the various `invoke` methods, but that might be more code than necessary. Then maybe something similar in `ClientProxyImpl.handleResponse`. Wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #851: CXF-8597: CXF JAXRS client not closing HTTP connections
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #851:
URL: https://github.com/apache/cxf/pull/851#discussion_r711065380
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -418,7 +418,7 @@ private Link makeAbsoluteLink(Link link) {
public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns)
throws ProcessingException, IllegalStateException {
- return doReadEntity(cls, t, anns, false);
+ return doReadEntity(cls, t, anns, !InputStream.class.isAssignableFrom(cls));
Review comment:
@andymc12 thanks a lot for your thoughts, indeed closing the response is the right thing to do but in case of client proxies, it is not explicit. Using finalizers could be an option but they are deprecated since JDK-9 [1] and are discouraged. I will look more closely into, it seems like we could isolate the client proxies, thank you
[1] https://bugs.openjdk.java.net/browse/JDK-8165641
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #851: CXF-8597: CXF JAXRS client not closing HTTP connections
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #851:
URL: https://github.com/apache/cxf/pull/851#discussion_r712581738
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -55,7 +55,6 @@
import javax.ws.rs.ext.ReaderInterceptor;
import javax.ws.rs.ext.RuntimeDelegate.HeaderDelegate;
import javax.xml.stream.XMLStreamReader;
-import javax.xml.transform.Source;
Review comment:
Should `javax.xml.transform.Source` be part of the `JAXRSUtils::isStreamingOutType` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #851: CXF-8597: CXF JAXRS client not closing HTTP connections
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #851:
URL: https://github.com/apache/cxf/pull/851#discussion_r710680169
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -418,7 +418,7 @@ private Link makeAbsoluteLink(Link link) {
public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns)
throws ProcessingException, IllegalStateException {
- return doReadEntity(cls, t, anns, false);
+ return doReadEntity(cls, t, anns, !InputStream.class.isAssignableFrom(cls));
Review comment:
@andymc12 may you please take a look? I suspect we may break some TCK tests but it seems like the method must close the entity (if `cls` not input stream) after consumption. Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #851: CXF-8597: CXF JAXRS client not closing HTTP connections
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #851:
URL: https://github.com/apache/cxf/pull/851#discussion_r710680169
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -418,7 +418,7 @@ private Link makeAbsoluteLink(Link link) {
public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns)
throws ProcessingException, IllegalStateException {
- return doReadEntity(cls, t, anns, false);
+ return doReadEntity(cls, t, anns, !InputStream.class.isAssignableFrom(cls));
Review comment:
@andymc12 may you please take a look? I suspect we may break some TCK tests but it seems like the method must close the entity (if not input stream) after consumption. Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] lkoe commented on a change in pull request #851: CXF-8597: CXF JAXRS client not closing HTTP connections
Posted by GitBox <gi...@apache.org>.
lkoe commented on a change in pull request #851:
URL: https://github.com/apache/cxf/pull/851#discussion_r710811961
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -418,7 +418,7 @@ private Link makeAbsoluteLink(Link link) {
public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns)
throws ProcessingException, IllegalStateException {
- return doReadEntity(cls, t, anns, false);
+ return doReadEntity(cls, t, anns, !InputStream.class.isAssignableFrom(cls));
Review comment:
the bug author here :-)
@andymc12 the use case in question is a simplistic JAXRS Proxy Client where user code really has no access to the response object.
Something like this:
```
public interface TestResource {
@POST
@Path("/test")
@Consumes(MediaType.APPLICATION_JSON)
@Produces(MediaType.APPLICATION_JSON)
TestModel test(TestModel body);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #851: CXF-8597: CXF JAXRS client not closing HTTP connections
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #851:
URL: https://github.com/apache/cxf/pull/851#discussion_r712580437
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -418,7 +418,7 @@ private Link makeAbsoluteLink(Link link) {
public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns)
throws ProcessingException, IllegalStateException {
- return doReadEntity(cls, t, anns, false);
+ return doReadEntity(cls, t, anns, !InputStream.class.isAssignableFrom(cls));
Review comment:
@andymc12 I think I nailed it down. In refactoring, we have lost the semantics of the `response.stream.auto.close` property (it was only taking into account in some specific flow). Also, I think the default auto-close detection mechanism is good enough to not requiring manual should/should not close instructions. The builds are green, what do you think about this one? Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta closed pull request #851: CXF-8597: CXF JAXRS client not closing HTTP connections
Posted by GitBox <gi...@apache.org>.
reta closed pull request #851:
URL: https://github.com/apache/cxf/pull/851
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] andymc12 commented on a change in pull request #851: CXF-8597: CXF JAXRS client not closing HTTP connections
Posted by GitBox <gi...@apache.org>.
andymc12 commented on a change in pull request #851:
URL: https://github.com/apache/cxf/pull/851#discussion_r710708460
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -418,7 +418,7 @@ private Link makeAbsoluteLink(Link link) {
public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns)
throws ProcessingException, IllegalStateException {
- return doReadEntity(cls, t, anns, false);
+ return doReadEntity(cls, t, anns, !InputStream.class.isAssignableFrom(cls));
Review comment:
Hmm... My take on this is that the user should be closing their `Response` objects (which are `AutoCloseable` making it easier to do so). Despite my comment to the contrary, the `readEntity` methods are not supposed to close the `Response` - they are supposed to close the underlying entity stream. Maybe we could fix this by always closing the entity stream in the finally block?
FWIW, RESTEasy uses a `finalize()` method on their `Response` implementation class that will close the response once garbage collected as a safety net. That might also work here. Wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #851: CXF-8597: CXF JAXRS client not closing HTTP connections
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #851:
URL: https://github.com/apache/cxf/pull/851#discussion_r710678426
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -418,7 +418,7 @@ private Link makeAbsoluteLink(Link link) {
public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns)
throws ProcessingException, IllegalStateException {
- return doReadEntity(cls, t, anns, false);
+ return doReadEntity(cls, t, anns, true);
Review comment:
@andymc12 may you please take a look? I suspect we may break some TCK tests but it seems like the method must close the entity (if input stream) after consumption. Thank you!
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -418,7 +418,7 @@ private Link makeAbsoluteLink(Link link) {
public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns)
throws ProcessingException, IllegalStateException {
- return doReadEntity(cls, t, anns, false);
+ return doReadEntity(cls, t, anns, !InputStream.class.isAssignableFrom(cls));
Review comment:
@andymc12 may you please take a look? I suspect we may break some TCK tests but it seems like the method must close the entity (if input stream) after consumption. Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #851: CXF-8597: CXF JAXRS client not closing HTTP connections
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #851:
URL: https://github.com/apache/cxf/pull/851#discussion_r710678426
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -418,7 +418,7 @@ private Link makeAbsoluteLink(Link link) {
public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns)
throws ProcessingException, IllegalStateException {
- return doReadEntity(cls, t, anns, false);
+ return doReadEntity(cls, t, anns, true);
Review comment:
@andymc12 may you please take a look? I suspect we may break some TCK tests but it seems like the method must close the entity (if input stream) after consumption. Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta merged pull request #851: CXF-8597: CXF JAXRS client not closing HTTP connections
Posted by GitBox <gi...@apache.org>.
reta merged pull request #851:
URL: https://github.com/apache/cxf/pull/851
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org