Fix memory leak when using ktor-client-java #4637
Conversation
laurit
requested review from
anuraaga,
iNikem,
jkwatson,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
| @@ -30,7 +30,7 @@ public BodyHandlerWrapper(BodyHandler<T> delegate, Context context) { | |||
| if (subscriber instanceof BodySubscriberWrapper) { | |||
| return subscriber; | |||
| } | |||
| return new BodySubscriberWrapper<>(delegate.apply(responseInfo), context); | |||
| return new BodySubscriberWrapper<>(subscriber, context); | |||
There was a problem hiding this comment.
Can you please explain, why the previous version caused the memory leak?
There was a problem hiding this comment.
On the line before the diff there is BodySubscriber<T> subscriber = delegate.apply(responseInfo); Apparently in case of ktor the result of delegate.apply which is a JavaHttpResponseBodySubscriber https://github.com/ktorio/ktor/blob/main/ktor-client/ktor-client-java/jvm/src/io/ktor/client/engine/java/JavaHttpResponseBodyHandler.kt can't just be thrown away without closing it. Anyway I believe that I'm the author of this bug, it should have looked like this from the start.
|
any changes of having this backported to 1.7.x? |
Considering that the release of 1.9 is imminent (yeah there is a jump from 1.7 to 1.9) any more releases in 1.7 series are unlikely. |
|
when is it planned for? on our perspective, it's always harder to pitch for a major upgrade in comparison to a bug fix one, so it would be nice to have such a safe change backported. let me know if there is anything I can do to make it happen. |
|
this issue does fit our patch release criteria, so I have added it to tomorrow's SIG meeting agenda to discuss 1.9.0 is targeted for end of this week, but currently looking likely to slip into next week |
|
Thank you very much for chasing that :) |
|
@brunojcm I tried to release a patch, but our patch release workflow is broken https://github.com/open-telemetry/opentelemetry-java-instrumentation/runs/4268334982?check_suite_focus=true, I think because maven snapshots were recently pruned https://oss.sonatype.org/content/repositories/snapshots/io/opentelemetry/javaagent/opentelemetry-javaagent/, and our build process was not resilient to that. The problem is solvable in the 1.7.x branch, but I'm not sure it's the best use of time given that we've done a lot of work in the upcoming release to improve our build process (in particular #4248) which should avoid this issue (and others we've had related to releasing patches) going forwards. |
|
No worries, thanks for trying, @trask! Looking forward to any release containing this fix, I can do the extra work on my side to do a major update, I'd have to do it eventually anyway. |
|
@brunojcm 1.9.0 is released, thanks again for reporting this! |
|
thank you very much, @trask, I'll give it a try today! |
Resolves #4629