Always close HTTP responses in HttpSender #571
Conversation
The Response object wasn't closed when the response is successful, leading to connection leaks. This was revealed by seeing messages like this in logs: WARNING: A connection to https://<redacted>/ was leaked. Did you forget to close a response body? Signed-off-by: Maxime Petazzoni <max@signalfx.com>
|
I'm not sure why this test is failing: https://travis-ci.org/jaegertracing/jaeger-client-java/jobs/462005629#L1693 Any help here is appreciated. |
|
Looks like something got stuck somewhere, as I don't think it's related to this PR. I just started the job again, let's see how it goes. |
Codecov Report
@@ Coverage Diff @@
## master #571 +/- ##
==========================================
+ Coverage 89.49% 89.8% +0.3%
- Complexity 539 542 +3
==========================================
Files 68 68
Lines 1942 1942
Branches 249 249
==========================================
+ Hits 1738 1744 +6
+ Misses 130 126 -4
+ Partials 74 72 -2
Continue to review full report at Codecov.
|
|
|
||
| String exceptionMessage = String.format("Could not send %d spans, response %d: %s", | ||
| spans.size(), response.code(), responseBody); | ||
| throw new SenderException(exceptionMessage, null, spans.size()); |
There was a problem hiding this comment.
Don't you need to close, in case the response isn't successful? Is there an alternative where try-with-resources is used, so that our code wouldn't need to care about the closing of the resources?
There was a problem hiding this comment.
Calling response.body().string() will close the response, so this happens fine in the non-successful code path.
Ideally we'd use a try-with-resources, but we can't use it because this project requires Java 6 compatibility.
There was a problem hiding this comment.
Can't wait to get rid of Java 6 support... (#511)
|
Thanks! Would you consider doing a 0.32.1 with this fix? If not, when do you think the next release train will pick it up? |
|
@pavolloffay can give you more info about releases, I'm a bit outdated on this area :) |
|
The release works how it is described in |
|
@jpkrohling Would you be willing to release a 0.32.1 following the |
|
Sorry, I'm on PTO until mid January. @objectiser or Pavol may help you with thatAm 13.12.2018 15:37 schrieb Maxime Petazzoni <notifications@github.com>:@jpkrohling Would you be willing to release a 0.32.1 following the RELEASE.md instructions? I'd like to be able to point people at an available release that doesn't leak connections.
—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@mpetazzoni @jpkrohling I've started the process in #575 - just waiting for @pavolloffay (at kubecon) or another @jaegertracing/jaeger-maintainers to approve. |
Using the jaeger trace exporter we started seeing `WARNING: A connection to http://jaeger:8080/ was leaked. Did you forget to close a response body? To see where this was allocated, set the OkHttpClient logger level to FINE` in our application logs. I traced this back to jaeger client release `0.33.0` that fixes this issue with closing the connection. jaegertracing/jaeger-client-java#571 https://github.com/jaegertracing/jaeger-client-java/releases
Using the jaeger trace exporter we started seeing `WARNING: A connection to http://jaeger:8080/ was leaked. Did you forget to close a response body? To see where this was allocated, set the OkHttpClient logger level to FINE` in our application logs. I traced this back to jaeger client release `0.33.0` that fixes this issue with closing the connection. The newer version of the jaeger client uses a new package name `io.jaegertracing` jaegertracing/jaeger-client-java#571 https://github.com/jaegertracing/jaeger-client-java/releases
* Bump jaeger trace exporter version Using the jaeger trace exporter we started seeing `WARNING: A connection to http://jaeger:8080/ was leaked. Did you forget to close a response body? To see where this was allocated, set the OkHttpClient logger level to FINE` in our application logs. I traced this back to jaeger client release `0.33.0` that fixes this issue with closing the connection. The newer version of the jaeger client uses a new package name `io.jaegertracing` jaegertracing/jaeger-client-java#571 https://github.com/jaegertracing/jaeger-client-java/releases * Fix import control
The Response object wasn't closed when the response is successful,
leading to connection leaks. This was revealed by seeing messages like
this in logs:
Signed-off-by: Maxime Petazzoni max@signalfx.com
Which problem is this PR solving?
Short description of the changes