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

Streaming bodies 3 #2754

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

trumpetinc
Copy link

Improving request and response handling to use streams instead of in-memory byte arrays. Streams are retriable (up to some amount of reading).

To demonstrate this streaming functionality, I have added decoders for handling typical stream-oriented body types (see /core/src/main/java/feign/stream/InputStreamAndReaderDecoder.java) - I suspect that eventually these encoders/decoders will be broken out of the core library.

Current state of the branch: Response streaming has been implemented.

Next: Add support for request streaming.

Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a most welcome change.

My sole concern is it changes a few APIs, so for me this is a major release
@kdavisk6

Seems build is not passing, please take a look =)

Great work

Comment on lines 16 to 29
/*
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a double license here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - I am not following - I think this is the license block in all of the Feign core source files?

Copy link
Member

@kdavisk6 kdavisk6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, with only one, minor suggestion.

And yes, this would need to be included in a major version change.

@trumpetinc
Copy link
Author

@kdavisk6 @velo

I have made the requested changes.

I want to make sure we are on the same page about the state of this PR before you accept it.

Right now:

  1. Multipart MIME requests really should be updated to take advantage of the new streaming functionality.

  2. The only client that I have updated is Client.Default. I think it would be a very good idea to make sure that the other clients can be reworked (if not actually rework them). If we don't rework a client, then the original Feign behavior (loading everything into memory) will be in effect for requests (responses will be fine).

Sometime in the next couple of days, I will review the following Client implementations and share my thoughts - I'm a little worried that some of these Client implementations assume that requests will be provided as InputStreams, which is not compatible with the new approach I am proposing (converting an outputstream to an inputstream requires a separate worker thread).

I'm confident that I can make this work with JAX-RS and Apache.
I'm pretty sure that I can make java11 Http2Client work.
I am not confident that I can make the Google client work, we will see.
I don't program in Kotlin, so I can't comment on OkHttpClient at all.

So there may be some philosophical discussion required here.

@Sung-Heon
Copy link

May I try pr for OkHttpClient?

@trumpetinc
Copy link
Author

@Sung-Heon

May I try pr for OkHttpClient?

I am still evaluating all of the various Client implementations to make sure that the current approach (requests using OutputStream) will work. So I have not updated OkHttpClient for the new functionality (and probably won't until I finish the evaluation to make sure the approach I've implemented will work).

That said, I just checked, and it should be straightforward to adjust OkHttpClient to support streaming.

For streaming requests, we would need to create our own implementation of RequestBody and override writeTo() that would call OutputStreamSender.sendToOutputStream(sink.outputStream()). Without this change, OkHttpClient will effectively have the original behavior of loading the entire request into memory before sending it.

Streaming responses are easier to implement - it actually should work with the existing code. I still want to make one change to OkHttpClient.toBody() to have it properly apply the header charset in the asReader() method, but that's a small thing.

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.

4 participants