playframework / play-ws

Standalone Play WS, an async HTTP client with fluent API
https://www.playframework.com/documentation/latest/JavaWS
Apache License 2.0
223 stars 87 forks source link

scala StandaloneWSRequest.stream() buffers entire response in memory #802

Closed ffettinger closed 1 year ago

ffettinger commented 1 year ago

As of version 2.2.0, calling the scala API StandaloneWSRequest.stream() now behaves the same as .execute(). Both buffer the entire response in memory because a StandaloneAhcWSResponse is returned. Previously, .stream() returned a StreamedResponse. The change in behavior was introduced by removing the call to .executeStream() in https://github.com/playframework/play-ws/commit/00c8ddb3ad278a774970ed0b10c58f04f8726517#diff-cc289ad1c53eab708b302d4dec006082cf9902504ccd91c198ad20de6319296eL239

Play WS Version 2.2.0

API Scala

Operating System ANY

JDK ANY

Expected Behavior

Calling StandaloneWSRequest.stream() followed by Response.bodyAsSource on the response should give the caller access to a stream of bytes which does not require buffering the entire response in memory.

Actual Behavior

As of version 2.2.0 StandaloneWSRequest.stream() and StandaloneWSRequest.execute() are essentially the same.

Memory usage increases as the response is processed until the following exception occurs:

Caused by: java.nio.BufferOverflowException
at java.base/java.nio.ByteBuffer.put(ByteBuffer.java:914)
at play.shaded.ahc.org.asynchttpclient.netty.NettyResponse.getResponseBodyAsBytes(NettyResponse.java:170)
at play.api.libs.ws.ahc.StandaloneAhcWSResponse.bodyAsBytes(StandaloneAhcWSResponse.scala:48)
at play.api.libs.ws.ahc.StandaloneAhcWSResponse.bodyAsSource(StandaloneAhcWSResponse.scala:50)

On 2.1.11 this exception does not occur.

Reproducible Test Case

I can't really think of an easy way to verify this by just checking the behavior unless there is a way to limit how much memory can be allocated to buffers during the test. I don't have the necessary expertise for that unfortunately. A test could make an assertion that the type returned by StandaloneWSRequest.stream() is StreamedResponse, but I'm not sure that is appropriate.

mkurz commented 1 year ago

@xuwei-k Since you are the original author of mentioned change, could you take a look when you have time?