Improve docs on AbstractStreamingClientHttpRequest for streaming vs buffering mode

[mdeinum]

Changes made in Spring 6.1 by removing buffering from the ClientHttpRequest implementations in this commit actually made all streaming implementations now buffering instead of streaming. This due to the use of the FastByteArrayOutputStream in the AbstractStreamingClientHttpRequest instead of the actual underlying OutputStream of the request.

The FastByteArrayOutputStream buffers the output which will lead to OOM errors when sending large payloads (as in the linked StackOverflow question).

See: https://stackoverflow.com/questions/79801951/spring-web-oom-when-streaming-after-update-from-6-0-23-to-version-6-1-4-and-abov
Related: #30557

Reproducer: https://github.com/Manveroo/spring-web-streaming

To use streaming the following RequestCallback implementation works.

  RequestCallback requestCallback =
    request -> {
      if (request instanceof StreamingHttpOutputMessage shom) {
            System.out.println("Using Body");
            shom.setBody(out ->
            {
              try (var fis = new BufferedInputStream(new FileInputStream(sourcePath.toFile()))) {
                doStream(fis, out);
              }
            });
          } else {
            System.out.println("Using OutputStream");
            try (var fis = new BufferedInputStream(new FileInputStream(sourcePath.toFile()))) {
              doStream(fis, request.getBody());
            }
          }
        };

However prior to 6.1.x just doing request.getBody() (the else branch) would work in a streaming way (if bufferng was set to false on the SimpleClientHttpRequestFactory.

[rstoyanchev]

Thanks for raising this. It is is a good question to consider.

This is indeed a consequence of the redesign in #30557. All request implementations now support streaming, but callers must be aware to use the request through the StreamingHttpOutputMessage contract. Or if getBody() is used, it falls back on buffering via ByteArrayOutputStream. In that sense, the decision to stream or now is in the hands of the caller whereas previously it was based on a configuration flag and fixed at runtime.

At this point I don't think it's worth bringing back the previous way of streaming, which was supported with the JDK HttpURLConnection and Apache HttpComponents. It would be confusing to have a flag to stream or to buffer, but the actual decision would also depend on the caller. It would lead to inconsistent behavior, e.g. streaming set to false, but caller uses StreamingHttpOutputMessage.

Mostly a transparent change since built-in code was changed at the same time, but a custom RequestCallback would need to be migrated to explicitly opt into streaming.

[mdeinum]

While I agree bringing back the previous way of streaming isn't the way forward. It is however confusing imho. The class is named AbstractStreamingClientHttpRequest where in fact it doesn't actually do streaming by default. So I can imagine that users are thrown off guard by this (especially with larger payloads, it probably won't even show with smallish payloads).

The least thing that could be done I think is to document this clearer (either Javadoc or maybe a streaming section/paragraph in the reference guide).

[rstoyanchev]

The class does support streaming, but only if used via StreamingHttpOutputMessage, and that could be more clearly explaned in the Javadoc of the class itself to make it more readily visible. An update in the reference documentation on this topic would also help.

Thanks for bringing this up.