Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify ClientHttpRequestInterceptor#intercept() must close response if it throws exception after receiving the response #29751

Closed
frederic-boissiere opened this issue Dec 29, 2022 · 6 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: documentation A documentation task
Milestone

Comments

@frederic-boissiere
Copy link

frederic-boissiere commented Dec 29, 2022

Affects: Spring Framework 5.3.22


Hello, our application implements a custom RestTemplate using org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager.

Moreover, a custom ClientHttpRequestInterceptor is added to this RestTemplate.

In case an exception is raised during the execution of the ClientHttpRequestInterceptor#intercept() method then the connection stays in leased state, and it is never released leading to "Timeout waiting for connection from pool" exception.

In attachment a simple sample reproducing this behavior: connpool.zip

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 29, 2022
@sbrannen sbrannen changed the title PoolingHttpClientConnectionManager connection leaks in case exception thrown during execution of ClientHttpRequestInterceptor#intercept() PoolingHttpClientConnectionManager connection leaks in case exception thrown during execution of ClientHttpRequestInterceptor#intercept() Jan 31, 2023
@sbrannen sbrannen added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jan 31, 2023
@snicoll
Copy link
Member

snicoll commented Oct 2, 2023

In attachment a simple sample reproducing this behavior:

@frederic-boissiere thanks for the sample but running the main invokes your custom code ten times and then exit. What part specifically is showcasing the connection leak?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Oct 2, 2023
@frederic-boissiere
Copy link
Author

frederic-boissiere commented Oct 2, 2023

Hello @snicoll,
in order to reproduce the case, you have to start the "server application" using "python3 dumb-server.py"
then the following message will appear in the java console during execution of the main java application : 16:27:40.760 [main] ERROR com.test.CustomRestTemplate - Exception: org.springframework.web.client.ResourceAccessException: I/O error on GET request for "http://localhost:8080/": Timeout waiting for connection from pool; nested exception is org.apache.http.conn.ConnectionPoolTimeoutException: Timeout waiting for connection from pool

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 2, 2023
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Oct 4, 2023

Normally, the response would be closed (and connection released) in RestTempate#doExecute, but if an interceptor throws an exception after the response comes back, then the rest of the execution chain is shortcircuited and doesn't get to see the response, and therefore can't close it. For this case you'll need to ensure the interceptor doesn't throw an exception, or otherwise close the response from the interceptor.

@rstoyanchev rstoyanchev closed this as not planned Won't fix, can't repro, duplicate, stale Oct 4, 2023
@rstoyanchev rstoyanchev added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Oct 4, 2023
@frederic-boissiere
Copy link
Author

Hello @rstoyanchev, from my point of view, this information is missing from the javadoc.
The signature of the method is not clear regarding this case : ClientHttpResponse intercept(HttpRequest request, byte[] body, ClientHttpRequestExecution execution) throws IOException; because it allows the propagation of IOException.

@rstoyanchev
Copy link
Contributor

Fair enough, I'll update the Javadoc.

@rstoyanchev rstoyanchev reopened this Oct 4, 2023
@rstoyanchev
Copy link
Contributor

By the way, looking at the custom RestTemplate, one thing it does is to prepare the URL. I'll mention that there is a puggable UriTemplateHandler that you could use for similar purposes in case you haven't noticed it.

@rstoyanchev rstoyanchev self-assigned this Oct 4, 2023
@rstoyanchev rstoyanchev added type: documentation A documentation task and removed status: invalid An issue that we don't feel is valid labels Oct 4, 2023
@rstoyanchev rstoyanchev added this to the 6.1.0-RC1 milestone Oct 4, 2023
@rstoyanchev rstoyanchev changed the title PoolingHttpClientConnectionManager connection leaks in case exception thrown during execution of ClientHttpRequestInterceptor#intercept() Clarify ClientHttpRequestInterceptor#intercept() must close response if it throws exception after receiving the response Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: documentation A documentation task
Projects
None yet
Development

No branches or pull requests

5 participants