retrofit icon indicating copy to clipboard operation
retrofit copied to clipboard

Stream bodies from first-party converters

Open JakeWharton opened this issue 6 years ago • 7 comments

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.

JakeWharton avatar Sep 28 '19 03:09 JakeWharton

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 avatar Oct 01 '19 07:10 ZacSweers

Eh just went ahead and opened a PR https://github.com/square/retrofit/pull/3220

ZacSweers avatar Oct 01 '19 07:10 ZacSweers

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?

efrohnhoefer avatar Nov 25 '24 18:11 efrohnhoefer

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.

pyricau avatar Nov 25 '24 19:11 pyricau

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 avatar Nov 25 '24 19:11 JakeWharton

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.

JakeWharton avatar Nov 25 '24 19:11 JakeWharton

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?

pyricau avatar Nov 26 '24 16:11 pyricau