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

Add ability to use InputStream as source of RequestBody #3912

Closed
wants to merge 1 commit into from

Conversation

yfouquet
Copy link

@yfouquet yfouquet commented Mar 5, 2018

Following #2424, support InputStream in RequestBody#create.

The implementation is taking feedbacks for last attempt: #1038.
As retries may be required:

  • we cannot close the input stream, it remains the responsability of the caller.
  • BufferedInputStream is used (instead of any InputStream) because it can be read several times.

BufferedInputStream is an InputStream that can be read several times.
Hence, provide a create methode for RequestBody that takes such stream as argument.
@swankjesse
Copy link
Collaborator

I would prefer to not add this because the performance consequences are so unexpected. Similarly, the fact that we can’t automatically close this stream is going to be surprising to callers.

It needs to load the entire request body into memory. I’d prefer that if a caller is writing a file, they just use a separate FileInputStream on each call to writeTo().

@yschimke
Copy link
Collaborator

yschimke commented Mar 6, 2018

I like this and think it is a great feature I could have used myself. It is, however, likely a source of support issues for the OkHttp maintainers because of the tricky edge cases. Maybe it is better in a single purpose library to publish from your own github project? The change seems pretty clean so it doesn't need to be part of okhttp core.

@yfouquet
Copy link
Author

yfouquet commented Mar 6, 2018

Thanks for your quick feedbacks @swankjesse and @yschimke.
I'll find an alternative.

@yfouquet yfouquet closed this Mar 6, 2018
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

Successfully merging this pull request may close these issues.

3 participants