Avoid closing response stream while downloading instance logs #14730
Conversation
| builder.contentLocation(uri); | ||
| builder.header(HttpHeaders.CONTENT_LENGTH, httpResponse.getEntity().getContentLength()); | ||
| return builder.build(); | ||
| CloseableHttpResponse httpResponse = _fileUploadDownloadClient.getHttpClient().execute(requestBuilder.build()); |
There was a problem hiding this comment.
It is not clear to me why this may end up closing the resource for other instances and specially it is not clear to me who is closing this closeable now.
There was a problem hiding this comment.
Yeah even I am not 100% sure but my understanding is that while controller is streaming log file from other instance its acting like a proxy and passing the same stream to the client/caller. Right now controller is closing the stream before client consumes it.
Maybe jersey is managing closing the stream as we don't handle it in /loggers/download flow as well.
There was a problem hiding this comment.
I'm also very confused. Does this change intentionally leak connection?
There was a problem hiding this comment.
Given these downloads may be large, we need to be sure we are cleaning these resources in a correct fashion. Alternatively we can dump the content of the stream to a file and use that file to stream data to the incoming request, but then we will need to be sure we end up deleting the file
There was a problem hiding this comment.
Updated the code to handle the connection properly. Thanks a lot @gortiz for helping out with the right approach!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14730 +/- ##
============================================
+ Coverage 61.75% 63.73% +1.98%
- Complexity 207 1611 +1404
============================================
Files 2436 2708 +272
Lines 133233 151225 +17992
Branches 20636 23345 +2709
============================================
+ Hits 82274 96385 +14111
- Misses 44911 47597 +2686
- Partials 6048 7243 +1195
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Description
Due to the response stream being closed on controller, client is unable to download the logs from other instances and facing