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

Stream bodies from first-party converters #3215

Open
JakeWharton opened this issue Sep 28, 2019 · 7 comments
Open

Stream bodies from first-party converters #3215

JakeWharton opened this issue Sep 28, 2019 · 7 comments
Labels

Comments

@JakeWharton
Copy link
Collaborator

We currently do not to guard against mutable inputs, but that doesn't seem worth optimizing for. Plus it defers even more work to the HTTP thread.

@ZacSweers
Copy link
Contributor

Looking at a PR for this but want to make sure I understand it correctly. Using Moshi as an example - should it work like this?

@Override public RequestBody convert(final T value) {
  return new RequestBody() {
    @Override public MediaType contentType() {
      return MEDIA_TYPE;
    }

    @Override public void writeTo(BufferedSink sink) throws IOException {
      try (JsonWriter writer = JsonWriter.of(sink)) {
        adapter.toJson(writer, value);
      }
    }
  };
}

@ZacSweers
Copy link
Contributor

Eh just went ahead and opened a PR #3220

@efrohnhoefer
Copy link

Given the current state of Retrofit, does it make sense to revisit this and create factory methods that will expose the ability to stream request bodies?

@pyricau
Copy link
Member

pyricau commented Nov 25, 2024

In this comment:

don't see anything that really needs changed in the API. You can already do everything on the background thread if you want to. We simply don't today.

But then here:

The Content-Length header is no longer set and if you're doing any kind of request signature (such as those required by various AWS usage) it might up and break your whole setup.

@JakeWharton The current behavior & API means you have to choose between "serializing lazily from the http thread" and "doing any kind of request signature".

I seems to me it'd be beneficial to support both. What we really want is:

  • Still do the body serializing first so that we can do work like request signature in interceptors
  • Do that work from a background thread rather than the caller thread.

We don't necessarily need an API change to support this, but we'd definitely at least need an behavior change (or the option to do so), e.g. having the converter keep the same API but be called from a background thread.

@JakeWharton
Copy link
Collaborator Author

Retrofit has no threading so it is not possible to do both. Factory overloads to switch to streamed bodies instead of buffered bodies is really the only option.

@JakeWharton
Copy link
Collaborator Author

Depending on what execution mechanism you were using, it may be possible to apply threading earlier through a custom CallAdapter in which you brought you own thread, updated the streaming body to a buffered one, and also executed it. It has to be an execution mechanism where you can return something immediately to the caller, so like Rx, maybe coroutines, futures, etc.

That's probably going to be pretty gross.

However, an interceptor could also do this if someone really needed it. You build a new request with the original request body written to a Buffer.

@pyricau
Copy link
Member

pyricau commented Nov 26, 2024

This approach to fixing the converters is interesting: https://gist.github.com/cbeyls/df12d51c427541eaffe1f6686e5480e2

It seems like this would fix the issue of not having content length, while making sure that's still computed on the http thread?

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

Successfully merging a pull request may close this issue.

4 participants