New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix grpc-web trailer rendering for fast path #1552
Conversation
runtime/src/main/scala/akka/grpc/internal/GrpcProtocolWeb.scala
Outdated
Show resolved
Hide resolved
Any plan to merge this in? This support is really important in my project. Otherwise my client will be broken because of the missing trailer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrudolph I added the necessary MiMa filter, is there anything more you were planning to look into here because you marked it as 'Draft'?
6a52a87
to
ab76809
Compare
I reworked the code. I tested against akka-grpc-quickstart-scala and looked at the output which now looks reasonable when queried like this:
|
While with 2.1.3 the output is this, i.e. missing the trailer completely:
|
@inline | ||
private final def encodeTrailer(trailer: Seq[HttpHeader]): ByteString = | ||
ByteString(trailer.mkString("", "\r\n", "\r\n")) | ||
private final def encodeTrailerHeaders(trailerHeaders: Iterator[(String, String)]): ByteString = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More code is now reused between strict and streamed responses.
case DataFrame(data) => | ||
AbstractGrpcProtocol.encodeFrameData(codec.compress(data), codec.isCompressed, isTrailer = false) | ||
case TrailerFrame(trailer) => | ||
AbstractGrpcProtocol.encodeFrameData(encodeTrailer(trailer), codec.isCompressed, isTrailer = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line might be another bug I introduced in the optimization: we potentially report compression but don't actually do compression.
Would be nice if you could have another look at the implementation, @raboof, given that we have no tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, haven't tested it myself yet
Co-authored-by: Arnout Engelen <arnout@engelen.eu>
@@ -87,7 +87,7 @@ object GrpcProtocol { | |||
messageEncoding: Codec, | |||
/** Encodes a frame as a part in a chunk stream. */ | |||
encodeFrame: Frame => ChunkStreamPart, | |||
/** A shortcut to encode a data frame directly into a ByteString */ | |||
/** A shortcut to encode a data frame directly into a Response */ | |||
encodeDataToResponse: (ByteString, immutable.Seq[HttpHeader], Trailer) => HttpResponse, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrudolph I think Trailer could get some optimizations as well—having a cached instance for an empty Trailer, and avoiding creating a collection of 1 element and then immediately creating a new collection via map: https://github.com/akka/akka-http/blob/main/akka-http-core/src/main/scala/akka/http/scaladsl/model/Trailer.scala#L33
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For akka-grpc we are already using a cached instance here: https://github.com/jrudolph/akka-grpc/blob/efda1b1ff4a8fd9849be66a5cd8c66369f6027af/runtime/src/main/scala/akka/grpc/internal/GrpcResponseHelpers.scala#L32. For akka-http itself, you could also just omit the trailer when not needed.
Turns out we lost trailer rendering in the performance optimization for strict entities. This is an attempt at adding it back. Would still be good to add some tests. I currently don't have time to develop this further but maybe we can prepare the fix for the next release.
Refs #1465