Move body writing logic into http-server #11342
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The main goal of this PR is to finally move the last pieces of the response encoding logic from RoutingInBoundHandler into a server-independent class (ResponseLifecycle). To accomplish this, I did a few refactors, mostly related to streaming responses. Before this patch, streaming responses (e.g. Publisher<MyBean>) were handled by using the MessageBodyWriter to encode each item into a netty HttpContent (fully buffered) and concatenating them into a single netty ByteBody, with optional JSON separators (start/end bracket and commas). The new approach is a bit different. The individual pieces are written using a new ResponseBodyWriter.writePiece method that, unlike the normal write method used before, does not have mutable response headers, which don't make sense for this scenario (the headers are already sent at the time of serialization). Additionally, it returns a ByteBody, so the piece can actually be streamed instead of fully buffered. Next, the ByteBodys are concatenated using a new ConcatenatingSubscriber that streams each piece and forwards the data into a streaming ByteBody depending on runtime (BaseSharedBuffer implementation, for netty that's StreamingNettyByteBody). This class is the most intricate new code of this PR. In order for servlet to use this ConcatenatingSubscriber, it needs a BaseSharedBuffer implementation, so I've moved the existing ReactiveByteBufferByteBody from the JDK client module to the HTTP module. For netty this is not used. After these changes, I was able to move all the encoding methods to ResponseLifecycle with few changes. There are only a few netty-specific overrides in NettyResponseLifecycle: Netty-based ConcatenatingSubscribers, and handling for the netty-specific StreamedHttpResponse. To replace some of the netty-specific optimizations in RoutingInBoundHandler (such as the NettyResponseBodyWriterWrapper which has been removed), I've created a new ByteBodyFactory class that expands on the ByteBufferFactory concept with body factory methods. Unlike ByteBufferFactory, each method has a clear default implementation that works just fine and will be used as-is by servlet, but the class is also designed so that each method has a slightly more efficient netty implementation. ResponseBodyWriter has been changed to use ByteBodyFactory instead of ByteBufferFactory, making NettyResponseBodyWriterWrapper obsolete and also making many ResponseBodyWriters netty-independent (though I haven't moved them to http-server yet).
yawkat
commented
Nov 14, 2024
| * @author Jonas Konrad | ||
| */ | ||
| @Internal | ||
| final class ByteBodySubscriber implements HttpResponse.BodySubscriber<CloseableByteBody>, BufferConsumer.Upstream { |
There was a problem hiding this comment.
FYI: this has simply been moved from ReactiveByteBufferByteBody where it was an inner class previously, it's not new code.
yawkat
added a commit
to micronaut-projects/micronaut-servlet
that referenced
this pull request
Nov 20, 2024
Instead of servlet-specific body encoding logic, use the ResponseLifecycle introduced in core by micronaut-projects/micronaut-core#11342 (pending review). Then we only have to take care of writing the ByteBody to the servlet response. Some other related changes: - Improved async reading/writing support, deprecate StreamedServletMessage - Rework HttpResponse lifecycle management: Core mostly expects HttpResponseFactory to return new, independent responses, but servlet returned views of the same ServletHttpResponse each time (with shared headers and such). New approach is to have only the latest HttpResponse backed by the real ServletHttpResponse, any previous responses are copied snapshots - Deprecate and stop using ServletResponseEncoder, it is replaced by core ResponseBodyEncoder. These changes fix all the remaining TCK failures that relate to response handling, except for FilterProxyTest.
dstepanov
reviewed
Nov 20, 2024
| @NonNull HttpRequest<?> request, | ||
| @NonNull MutableHttpResponse<T> httpResponse, | ||
| @NonNull Argument<T> type, | ||
| @NonNull MediaType mediaType, | ||
| T object) throws CodecException; | ||
|
|
||
| /** | ||
| * Write a <i>piece</i> of a larger response, e.g. when writing a Publisher or a part of a |
There was a problem hiding this comment.
What exactly is a piece of the response? A piece of the body? Should it be writebodyPart? What is the relation to the returned closeable body? How is supposed to close it?
There was a problem hiding this comment.
A "piece" can be a part (as in multipart), but right now it's only a piece of a streamed response (e.g. when you have a streamed json array). I called it piece instead of part to avoid giving the impression that this is only for multipart, though I plan to use this for multipart as well.
The returned body is Closeable because ownership transfers to the caller. The caller has to either close the body or use it (e.g. concatenating for the json array use case).
There was a problem hiding this comment.
is Chunk a better name? That is what Netty refers to it is it not?
There was a problem hiding this comment.
"chunk" is already used in the context transfer-encoding: chunked, so it is not really appropriate either
dstepanov
reviewed
Nov 20, 2024
| * @since 3.1.0 | ||
| */ | ||
| @ConfigurationProperties("responses.file") | ||
| public static class FileTypeHandlerConfiguration { |
There was a problem hiding this comment.
I'm just moving the micronaut.server.netty.responses.file config to micronaut.server.responses.file, not sure if this is the best way
|
graemerocher
requested changes
Nov 29, 2024
http-netty/src/main/java/io/micronaut/http/netty/body/NettyByteBodyFactory.java
Show resolved
Hide resolved
http-netty/src/main/java/io/micronaut/http/netty/body/NettyWritableBodyWriter.java
Outdated
Show resolved
Hide resolved
http-netty/src/main/java/io/micronaut/http/netty/body/NettyWritableBodyWriter.java
Outdated
Show resolved
Hide resolved
http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyResponseLifecycle.java
Outdated
Show resolved
Hide resolved
http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyResponseLifecycle.java
Show resolved
Hide resolved
http/src/main/java/io/micronaut/http/body/ByteBufferBodyAdapter.java
Outdated
Show resolved
Hide resolved
http/src/main/java/io/micronaut/http/body/ByteBufferBodyAdapter.java
Outdated
Show resolved
Hide resolved
| @NonNull HttpRequest<?> request, | ||
| @NonNull MutableHttpResponse<T> httpResponse, | ||
| @NonNull Argument<T> type, | ||
| @NonNull MediaType mediaType, | ||
| T object) throws CodecException; | ||
|
|
||
| /** | ||
| * Write a <i>piece</i> of a larger response, e.g. when writing a Publisher or a part of a |
There was a problem hiding this comment.
is Chunk a better name? That is what Netty refers to it is it not?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The main goal of this PR is to finally move the last pieces of the response encoding logic from RoutingInBoundHandler into a server-independent class (ResponseLifecycle). To accomplish this, I did a few refactors, mostly related to streaming responses.
Before this patch, streaming responses (e.g. Publisher) were handled by using the MessageBodyWriter to encode each item into a netty HttpContent (fully buffered) and concatenating them into a single netty ByteBody, with optional JSON separators (start/end bracket and commas).
The new approach is a bit different. The individual pieces are written using a new ResponseBodyWriter.writePiece method that, unlike the normal write method used before, does not have mutable response headers, which don't make sense for this scenario (the headers are already sent at the time of serialization). Additionally, it returns a ByteBody, so the piece can actually be streamed instead of fully buffered.
Next, the ByteBodys are concatenated using a new ConcatenatingSubscriber that streams each piece and forwards the data into a streaming ByteBody depending on runtime (BaseSharedBuffer implementation, for netty that's StreamingNettyByteBody). This class is the most intricate new code of this PR.
In order for servlet to use this ConcatenatingSubscriber, it needs a BaseSharedBuffer implementation, so I've moved the existing ReactiveByteBufferByteBody from the JDK client module to the HTTP module. For netty this is not used.
After these changes, I was able to move all the encoding methods to ResponseLifecycle with few changes. There are only a few netty-specific overrides in NettyResponseLifecycle: Netty-based ConcatenatingSubscribers, and handling for the netty-specific StreamedHttpResponse.
To replace some of the netty-specific optimizations in RoutingInBoundHandler (such as the NettyResponseBodyWriterWrapper which has been removed), I've created a new ByteBodyFactory class that expands on the ByteBufferFactory concept with body factory methods. Unlike ByteBufferFactory, each method has a clear default implementation that works just fine and will be used as-is by servlet, but the class is also designed so that each method has a slightly more efficient netty implementation. ResponseBodyWriter has been changed to use ByteBodyFactory instead of ByteBufferFactory, making NettyResponseBodyWriterWrapper obsolete and also making many ResponseBodyWriters netty-independent (though I haven't moved them to http-server yet).