BlockingStreamingHttpService: drop trailers if users didn't create any

[idelpivnitskiy]

Motivation:

Behavior was discovered as part of debugging #3148.
Currently, BlockingStreamingHttpService allocates trailers upfront and concatenates them after the payload body when protocol allows. In result, aggregation of BlockingStreamingHttpResponse does not lead to a single HTTP/2 frame because we always expect to receive trailers. However, adding trailers after users close payload writer is race. There are no guarantees that new trailers will be written to the network after close. Therefore, we can inspect the state after close and decide if we need to append trailers or not.

Modifications:

• Use scanWithMapper inside BlockingStreamingHttpService instead of always concatenating payload body with trailers;
• Create trailers on demand inside BufferHttpPayloadWriter, only when users touch them;
• Clarify behavior in HttpPayloadWriter's javadoc for trailers-related methods;
• Enhance BlockingStreamingToStreamingServiceTest to assert new expectations;

Result:

In BlockingStreamingHttpService, trailers are allocated and attached only if users touch trailers before closing HttpPayloadWriter.

[daschl]

What's the use-case of empty trailers?

[idelpivnitskiy]

Good question. Looking at it with a fresh morning look.

My original thinking was that if users didn't touch trailers at all, then it's absolutely safe to drop them. But if they did, they could do something like:

HttpHeaders trailers = writer.trailers();
....
trailers.add(name, value);

However, if they did that before close(), then we will see that. If they added smth after close(), it's anyway racy and not guaranteed.

Another thing is how we treat trailers in all other places:

1. DefaultHttpRequest.toStreamingRequest() or DefaultHttpResponse.toStreamingResponse()

servicetalk/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpRequest.java

Lines 277 to 283 in 6fc7987

	@Nullable
	final Publisher<Object> payload;
	if (trailers != null) {
	payload = emptyPayloadBody ? from(trailers) : from(payloadBody, trailers);
	} else {
	payload = emptyPayloadBody ? null : from(payloadBody);
	}

2. HttpDataSourceTransformations.aggregatePayloadAndTrailers:

servicetalk/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpDataSourceTransformations.java

Lines 340 to 348 in 6fc7987

	}).map(pair -> {
	if (isAlwaysEmpty(pair.payload)) {
	payloadInfo.setEmpty(true);
	}
	if (pair.trailers == null) {
	payloadInfo.setMayHaveTrailersAndGenericTypeBuffer(false);
	}
	return pair;
	});

Those account for the case when users can grab a reference to trailers, then convert the response and still use that reference to add trailers. Something like:

HttpResponse aggregatedResponse = ...;
HttpHeaders trailers = aggregatedResponse.trailers();
StreamingHttpResponse streamingResponse = aggregatedResponse.toStreamingResponse();
trailers.add(name, value);
return Single.succeeded(streamingResponse);

However, now I see that BlockingStreamingHttpServerResponse doesn't have conversion methods at all, and this trick won't be possible with HttpPayloadWriter anyway bcz we already sent meta-data.

The only real reason to keep it as a null check instead of "null or empty" is to keep it consistent with the above 2 cases. WDYT?

[mgodave]

I think keeping the cases consistent makes sense but if there is a way to simplify I would prefer that.

[idelpivnitskiy]

+1 for starting in a consistent way and if we feel different, we can update them all together

[daschl]

Thank you for the explanation, makes sense to me to keep it consistent for now 👍

[mgodave]

I think keeping the cases consistent makes sense but if there is a way to simplify I would prefer that.

[mgodave]

this seemed odd to me and was one thing I had trouble grokking when I was trying to track this down.

[idelpivnitskiy]

I agree 😞
Unfortunately, we currently don't have a nice way to update PayloadInfo flags without invoking transformation that will do it for us. Maybe we refactor in the future.

[mgodave]

A "getter" method that also creates seems off to me. IDK if there is a better naming scheme where trailers0 is basically just trailers and trailers is getOrCreateTrailers.

[idelpivnitskiy]

It's "lazy get" 😄
We use the same trick in AbstractHttpMetaData.context(). When API is written in stone but we need to defer action.
From users point of view it will be non-visible change.