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

fix #1385: Fix a response leak when requests are canceled #1415

Merged
merged 2 commits into from Jan 15, 2020

Conversation

carterkozak
Copy link
Contributor

This is a workaround for an upstream bug in:
https://github.com/square/okhttp/blob/d28d2cec21641b61f3d34e05dd52f43a717c2d32/okhttp/src/main/java/okhttp3/RealCall.java#L210-L213

I verified this using this reproducer before and after this change: https://github.com/palantir/conjure-java-runtime/compare/develop...ckozak/reproduce_leak?expand=1

Before this PR

Leaked connections and concurrency limiters. Services which frequently cancel requests observe substantially reduced throughput while waiting for resources to be reaped or garbage collected.

After this PR

==COMMIT_MSG==
Fix a response leak when requests are canceled. This is a workaround for an upstream bug in okhttp.
==COMMIT_MSG==

Possible downsides?

RPC is fragile, and can break just about everything else.

@changelog-app
Copy link

changelog-app bot commented Jan 15, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Fix a response leak when requests are canceled. This is a workaround for an upstream bug in okhttp.

Check the box to generate changelog(s)

  • Generate changelog entry

@ferozco
Copy link
Contributor

ferozco commented Jan 15, 2020

I think I prefer this approach to the one in #1414 since it should completely solve the leak and we're unlikely to make changes that would cause it to break in the short term. I basically view it as a patch that will tide us over until we get onto dialogue

@dansanduleac
Copy link
Contributor

@carterkozak can we actually make your reproducer into a test?

@carterkozak
Copy link
Contributor Author

Not reliably, unfortunately. Anything involving thread interruption results in super flaky tests (the last fix was almost trivial, and it still flaked a bunch). This requires precise timing that my reproducer had to brute force, and I'm not confident it will work reliably on all hardware.

@dansanduleac
Copy link
Contributor

dansanduleac commented Jan 15, 2020

👍 LGTM but probably good to have other people's +1 on this as well

@carterkozak
Copy link
Contributor Author

@dansanduleac anyone in particular you think I should reach out to?

@bulldozer-bot bulldozer-bot bot merged commit e192794 into develop Jan 15, 2020
@bulldozer-bot bulldozer-bot bot deleted the ckozak/response_leak branch January 15, 2020 19:03
@svc-autorelease
Copy link
Collaborator

Released 4.53.0

@dansanduleac
Copy link
Contributor

@carterkozak yeah never mind, just thought another person would be good just cos of this repo's complexity, didn't have anyone in particular in mind :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants