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

The OkHttp Response object is (partially) mutable #1

Closed
ynx0 opened this issue Nov 25, 2020 · 1 comment
Closed

The OkHttp Response object is (partially) mutable #1

ynx0 opened this issue Nov 25, 2020 · 1 comment

Comments

@ynx0
Copy link
Owner

ynx0 commented Nov 25, 2020

The OkHttp Response object is partially mutable. Specifically, the ResponseBody object is backed by a "one-shot" buffer that is responsible for storing the body content.

This is slightly problematic because:

  1. it makes for bad ergonomics. Although not the end of the world, I cannot, for example, print the response body within the sendMessage method and still have it available for the caller, because it is consumed.
  2. this behaviour is only present in the object returned by response.body(), but all other properties of the Response object are mutable. This mixed (im)mutability makes it hard to reason for an outsider about the mutability, making it error prone. in practice, this doesn't really matter cause I don't expect anyone else to really touch that area in the near future.
  3. if I return the Response object untouched, it leaves a potential memory leak because the responsibility is now on the caller to call .body() or .close() on the Response/ResponseBody object, which is probably not obvious to them.

One solution that came up in a discussion $todo:link$, was to create your own InMemoryResponseBody which does a zero-copy clone of the buffer and keeps it permanently in memory.
This solution, while doable, is unwieldy because it requires not only creating this custom type, but also creating a custom Response object (e.g. InMemoryResponse) that wraps around/contains this type instead of the regular ResponseBody type. If for some reason the new class cannot simply extend Response this also hinders resuability in case someone expects/needs an OkHttp Response and is instead given my custom class.

Overall, I will probably just have to experiment and try out this solution to see how bad it is. Maybe further along down the road I'll find a better solution.

@ynx0
Copy link
Owner Author

ynx0 commented Dec 1, 2020

Addressed in #9 with implementation of InMemoryResponseWrapper

@ynx0 ynx0 closed this as completed Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant