Converter API encourages blocking request body serialization on caller thread

[pyricau]

The Converter interface exposes RequestBody convert(T value) which encourages turning the body value into a ByteString based RequestBody.

For example, let's look at GsonRequestBodyConverter:

  @Override
  public RequestBody convert(T value) throws IOException {
    Buffer buffer = new Buffer();
    Writer writer = new OutputStreamWriter(buffer.outputStream(), UTF_8);
    JsonWriter jsonWriter = gson.newJsonWriter(writer);
    adapter.write(jsonWriter, value);
    jsonWriter.close();
    return RequestBody.create(MEDIA_TYPE, buffer.readByteString());
  }

When calling OkHttpCall enqueue APIs (usually from the main thread), we typically expect serialization work to happen on a background thread. We've recently realized that the convert method is called directly from the same thread where you call enqueue.

       at sun.nio.cs.StreamEncoder.write(StreamEncoder.java:111)
        at java.io.OutputStreamWriter.write(OutputStreamWriter.java:194)
        at com.google.gson.stream.JsonWriter.string(JsonWriter.java:617)
        at com.google.gson.stream.JsonWriter.value(JsonWriter.java:419)
        at com.google.gson.internal.bind.TypeAdapters$15.write(TypeAdapters.java:425)
        at com.google.gson.internal.bind.TypeAdapters$15.write(TypeAdapters.java:409)
        at com.google.gson.TypeAdapter$1.write(TypeAdapter.java:196)
        at com.squareup.wire.GsonJsonIntegration$ListJsonAdapter.write(GsonJsonIntegration.kt:108)
        at com.squareup.wire.GsonJsonIntegration$ListJsonAdapter.write(GsonJsonIntegration.kt:92)
        at com.google.gson.TypeAdapter$1.write(TypeAdapter.java:196)
        at com.squareup.wire.MessageTypeAdapter$write$1.invoke(MessageTypeAdapter.kt:51)
        at com.squareup.wire.MessageTypeAdapter$write$1.invoke(MessageTypeAdapter.kt:45)
        at com.squareup.wire.internal.RuntimeMessageAdapter.writeAllFields(RuntimeMessageAdapter.kt:218)
        at com.squareup.wire.MessageTypeAdapter.write(MessageTypeAdapter.kt:45)
        at com.squareup.wire.MessageTypeAdapter.write(MessageTypeAdapter.kt:25)
        at com.google.gson.TypeAdapter$1.write(TypeAdapter.java:196)
        at retrofit2.converter.gson.GsonRequestBodyConverter.convert(GsonRequestBodyConverter.java:47)
        at retrofit2.converter.gson.GsonRequestBodyConverter.convert(GsonRequestBodyConverter.java:31)
        at retrofit2.ParameterHandler$Body.apply(ParameterHandler.java:438)
        at retrofit2.RequestFactory.create(RequestFactory.java:131)
        at retrofit2.OkHttpCall.createRawCall(OkHttpCall.java:211)
        at retrofit2.OkHttpCall.enqueue(OkHttpCall.java:133)
        at retrofit2.KotlinExtensions.awaitResponse(KotlinExtensions.kt:97)
        at com.example.ExampleCode.startRequest(ExampleCode.kt:97)

@swankjesse called out that this was always the case, and that design decision avoids the risk of users passing in a mutable body request object that might get mutated while e.g. being json serialized in a background thread, which could cause all sorts of trouble.

We can avoid the serialization work by implementing a RequestBody that performs the serialization lazily:

    @Throws(IOException::class)
    override fun convert(value: T): RequestBody {
      return object : RequestBody() {
        override fun contentType() = MEDIA_TYPE

        override fun writeTo(sink: BufferedSink) {
          gson.newJsonWriter(OutputStreamWriter(sink.outputStream(), UTF_8)).use { jsonWriter ->
            adapter.write(jsonWriter, value)
          }
        }
      }
    }

Unfortunately, that's not what GsonRequestBodyConverter does. The following standard converters have the same issue: MoshiRequestBodyConverter, JaxbRequestConverter, WireRequestBodyConverter, SimpleXmlRequestBodyConverter.

While it would make sense to provide users with a choice of "slower+safer" or "faster+don't be dumb" option, I think we should default to the faster option and stop having all Android apps everywhere do request body serialization on the main thread.

And I wonder if the Converter API should evolve to make it more natural to write body serialization lazily and off the caller thread.

[JakeWharton]

Duplicate of #3215

[JakeWharton]

I 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.

There's already an issue for changing the built-in converters, but doing so impacts things like request signing so it can only be changed in a major version.

[efrohnhoefer]

We could update the converter factory to enable the use of streaming in a way the would enable experimentation and preserve the current defaults. Thoughts?

class GsonConverterFactory(
  private val useStreaming: Boolean = false,
  private val gson: Gson = Gson()
) : Converter.Factory() {
  override fun responseBodyConverter(
    type: Type,
    annotations: Array<out Annotation>,
    retrofit: Retrofit
  ): Converter<ResponseBody, *>? {
    val adapter: TypeAdapter<*> = gson.getAdapter(TypeToken.get(type))
    return GsonResponseBodyConverter<>(gson, adapter);
  }

  override fun requestBodyConverter(
    type: Type,
    parameterAnnotations: Array<out Annotation>,
    methodAnnotations: Array<out Annotation>,
    retrofit: Retrofit
  ): Converter<*, RequestBody>? {
   val adapter: TypeAdapter<*> = gson.getAdapter(TypeToken.get(type))
    return if (useStreaming) {
      StreamingGsonRequestBodyConverter(gson, adapter)
    } else {
      GsonRequestBodyConverter(gson, adapter)
  }

  internal class StreamingGsonRequestBodyConverter<T>(
    private val gson: Gson,
    private val adapter: TypeAdapter<T>,
  ) : Converter<T, RequestBody> {

    @Throws(IOException::class)
    override fun convert(value: T): RequestBody {
      return object : RequestBody() {
        override fun contentType() = MEDIA_TYPE

        override fun writeTo(sink: BufferedSink) {
          gson.newJsonWriter(OutputStreamWriter(sink.outputStream(), UTF_8)).use { jsonWriter ->
            adapter.write(jsonWriter, value)
          }
        }
      }
    }

    companion object {
      private val MEDIA_TYPE = "application/json; charset=UTF-8".toMediaType()
    }
  }
}

[JakeWharton]

Yeah, that's basically what we agreed upon earlier. See https://github.com/square/retrofit/pull/3220#issuecomment-695160988. Let's discuss on the open issue instead of this one, though.