TrailersSource, a new public API for HTTP trailers #8829
Conversation
This doesn't change much. The new interface replaces a Kotlin lambda that had similar behavior, though that lambda wasn't specified as thoroughly. We now permit calls to Response.trailers() before the entire response body is consumed. But when such calls are made the response body is discarded first. This could be a bit of a surprise to users, especially users who do an additional layer of buffering over their ResponseBody because it could yield latent failures. But I think it's better behavior overall, especially since it's possible data binding layers might not consume the entire response body in all cases. For example, a JSON library might not read to the very end of the response if it has received a terminating '}' byte. The riskiest part of this change is when ResponseBodySource self-reports as complete. Previously it would self-report as complete when it returned as many bytes as promised in the Content-Length. With this change it will now only report itself complete when it receives a definitive EOF on the stream, signaled by a -1 byte count on a read. This is because only when the EOF is received can we be sure that the trailers are received, and we must not unhook the Exchange from the Call until that happens. The previous behavior could make Call.cancel() no-op even if we were blocked waiting on trailers.
| @@ -13,6 +13,11 @@ | |||
| * See the License for the specific language governing permissions and | |||
| * limitations under the License. | |||
| */ | |||
| @file:Suppress( | |||
There was a problem hiding this comment.
Gradle is satisfied by the friends mechanism, but IntelliJ needs this also
| * call this without consuming the complete response body, any remaining bytes in the response | ||
| * body will be discarded before trailers are returned. | ||
| * | ||
| * If [Call.cancel] is called while this is blocking, this call will immediately throw. |
There was a problem hiding this comment.
keeping this promise was tricky!
| trailersAndCache(Protocol.HTTP_2) | ||
| } | ||
|
|
||
| private fun trailersAndCache(protocol: Protocol) { |
There was a problem hiding this comment.
TOTALLY DOESN’T WORK
|
|
||
| @Test | ||
| fun delayBeforeTrailersHttp1() { | ||
| delayBeforeTrailers(Protocol.HTTP_1_1) |
There was a problem hiding this comment.
This manual no-burst stuff is the worst. I haven’t done it here 'cause I’m afraid of how our MockWebServer JUnit 5 thing will interact with Burst.
There was a problem hiding this comment.
(More precisely, Burst is broken when you do fancy things with JUnit 5)
There was a problem hiding this comment.
Is that something to fix in Burst, Junit 5, or mws-ju5?
There was a problem hiding this comment.
I don’t know if we can fix it anywhere. Burst kind of assumes it can do its own naughty business with parameters, and it might not have enough information to leave the JUnit-5 managed ones alone.
| val trailers = response.trailers() | ||
| assertThat(trailers).isEqualTo(headersOf("t1", "v2")) | ||
| } | ||
| assertThat(trailersDelay).isGreaterThan(250.milliseconds) |
There was a problem hiding this comment.
Defending against weirdness like the client assuming there’s no trailers 'cause there’s no trailers yet
swankjesse
added
android-regression
okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http1/Http1ExchangeCodec.kt
Show resolved
Hide resolved
|
|
||
| @Test | ||
| fun delayBeforeTrailersHttp1() { | ||
| delayBeforeTrailers(Protocol.HTTP_1_1) |
There was a problem hiding this comment.
Is that something to fix in Burst, Junit 5, or mws-ju5?
|
To fix the tests I needed a to push a commit with non-trivial changes to @yschimke Please take a second look? |
One of our tests caught a situation where we were waiting for the caller to read the EOF when we didn't need to.
This doesn't change much.
The new interface replaces a Kotlin lambda that had similar behavior, though that lambda wasn't specified as thoroughly.
We now permit calls to Response.trailers() before the entire response body is consumed. But when such calls are made the response body is discarded first. This could be a bit of a surprise to users, especially users who do an additional layer of buffering over their ResponseBody because it could yield latent failures.
But I think it's better behavior overall, especially since it's possible data binding layers might not consume the entire response body in all cases. For example, a JSON library might not read to the very end of the response if it has received a terminating '}' byte.
The riskiest part of this change is when ResponseBodySource self-reports as complete. Previously it would self-report as complete when it returned as many bytes as promised in the Content-Length. With this change it will now only report itself complete when it receives a definitive EOF on the stream, signaled by a -1 byte count on a read. This is because only when the EOF is received can we be sure that the trailers are received, and we must not unhook the Exchange from the Call until that happens. The previous behavior could make Call.cancel() no-op even if we were blocked waiting on trailers.