Skip to content
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

New request body API #10781

Merged
merged 24 commits into from
May 27, 2024
Merged

New request body API #10781

merged 24 commits into from
May 27, 2024

Conversation

yawkat
Copy link
Member

@yawkat yawkat commented Apr 30, 2024

This PR adds a new public API for accessing the bytes of a request in the HTTP server. While it shares some naming with the ByteBody API we used before, it is a very different design. It's designed to be public API, not netty-specific, but still powerful enough to allow for the ByteBody optimizations we had before.

At the moment, the new api is only used for requests and only on the server. I'd like to expand it to the response and to the client if possible. But there are some challenges, so it won't make it into this PR:

  • ByteBody must be closed. Only NettyHttpRequest has the wiring to make that possible at the moment, the other HttpReq/Resp implementations are much more loose about resource management.
  • The server request is the only point where filters are actually executed when the body is in byte form, at the moment. In client messages and in the server response, the body is in object form when filters are executed.

Another missing piece is a non-netty implementation of ByteBody. I am particularly interested in a servlet implementation based on InputStream. I will implement that as a separate PR.

Some pieces of the old netty-only body api remain (the ObjectBody impls) to keep changes down. Also AbstractHttpContentProcessor is finally removed, all fields are folded into FormDataHttpContentProcessor.

@yawkat yawkat added the type: enhancement New feature or request label Apr 30, 2024
@yawkat yawkat added this to the 4.5.0 milestone Apr 30, 2024
Copy link
Contributor

@timyates timyates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sonar has some concerns with Null pointer exceptions

src/main/docs/guide/httpServer/byteBody.adoc Outdated Show resolved Hide resolved
src/main/docs/guide/httpServer/byteBody.adoc Outdated Show resolved Hide resolved
src/main/docs/guide/httpServer/byteBody.adoc Show resolved Hide resolved
Copy link
Contributor

@graemerocher graemerocher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like @dstepanov to review this when he is back

src/main/docs/guide/httpServer/byteBody.adoc Outdated Show resolved Hide resolved

While `ServerHttpRequest.byteBody()` returns a normal `ByteBody` -- cleanup is done by the HTTP server if the
body is not consumed--the body returned by `split` is a `CloseableByteBody`. The caller *must* ensure that the
new instance is closed, otherwise there can be resource and memory leaks, stalled connections, or other issues.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need a documented example of this use case and splitting

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the BodyLogFilter has splitting

* `SplitBackpressureMode.NEW` uses the backpressure of the new consumer (the one `split()` returns)

The argument-less `split()` method uses `SLOWEST`, but you should pick the mode that is most appropriate for your use
case.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again documentation needs example code

.onErrorComplete(ByteBody.BodyDiscardedException.class) // <7>
.subscribe(array -> LOG.info("Received body: {}", Base64.getEncoder().encodeToString(array))); // <8>
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide example of how to do this in the blocking case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cant really be implemented in a blocking way, because that would block the filter until the whole body is received, which we dont want for logging

@graemerocher
Copy link
Contributor

overall looks like a good change but think it needs some refinement to docs and clarification on blocking use cases.

@graemerocher
Copy link
Contributor

can you address the 6 new bugs identified by sonar

16:29:30.708 [default-nioEventLoopGroup-1-3] INFO i.m.docs.server.body.BodyLogFilter - Received body: ...
----

Note that the logging in the above example is asynchronous, so the log statements may be interleaved as shown.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way to do Flux.toList() to avoid this interleaving?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, i simply do not want to make guarantees about ordering. in fact the sentence and the example log are lying, there is no interleaving atm, but i dont want people to rely on that.

all subscribers to the same body have equal priority and may get parts of the body in any order.

"claim" the `byteBody()` and e.g. parse the JSON. Finally, the body is closed at the end of the request lifecycle,
discarding any data if it has not been claimed by the argument binder.

=== Primary operations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split in multiple files

Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should create

  • httpServer/byteBody/byteBodyPrimaryOperations.adoc
  • httpServer/byteBody/byteBodySplitting.adoc
  • httpServer/byteBody/byteBodyBackpressure.adoc
  • httpServer/byteBody/byteBodyDiscarding.adoc
  • httpServer/byteBody/byteBodyExample.adoc
  • httpServer/byteBody/byteBodyExample.adoc

src/main/docs/guide/httpServer/byteBody.adoc Outdated Show resolved Hide resolved
src/main/docs/guide/httpServer/byteBody.adoc Outdated Show resolved Hide resolved
src/main/docs/guide/httpServer/byteBody.adoc Outdated Show resolved Hide resolved
src/main/docs/guide/httpServer/byteBody.adoc Outdated Show resolved Hide resolved
@@ -707,25 +658,25 @@ public long getContentLength() {

@Override
public boolean isFull() {
return byteBody() instanceof ImmediateByteBody;
return byteBody() instanceof AvailableNettyByteBody;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we deprecate this and the contents methods and move them to the body interface?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the entire FullHttpRequest is now deprecated.

import io.micronaut.core.execution.CompletableFutureExecutionFlow;
import io.micronaut.core.execution.ExecutionFlow;
import io.micronaut.core.io.buffer.ByteBuffer;
import org.jetbrains.annotations.Contract;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we use jetbrains annotations

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's compile only so it shouldnt hurt

* performed on it
*/
@NonNull
CloseableByteBody split(@NonNull SplitBackpressureMode backpressureMode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the naming here. Why split? Should it be a copy with buffered data? So maybe buffered or something else? WDYT @graemerocher

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not necessarily buffered. it splits the stream so that there can be two consumers

*/
@Contract("-> this")
@NonNull
default ByteBody allowDiscard() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it affect the current instance? The name sounds more like a setter. I would name it discarded if only the returned instance is affected.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it affects the current instance

* @return The streamed bytes
*/
@NonNull
InputStream toInputStream();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should allow only using the input stream in lambda, which it will close: R read(is -> ...). And perhaps it would be user-friendly to have API like R readAndDiscard(is -> ...) + R readAndRetain(is -> ...) to identify directly what should happen to the data. WDYT @graemerocher

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those seem complementary and useful but there will be cases where the user needs to be in control when the input stream is closed I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would be the difference between discard and retain?

* @return The streamed bytes
*/
@NonNull
Publisher<byte[]> toByteArrayPublisher();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe these two should be moved to some StreamingByteBody interface?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is intentional that ImmediateByteBody extends the streaming case, so that users write their code for the streaming case but we can give optimized implementations where possible.

http/src/main/java/io/micronaut/http/body/ByteBody.java Outdated Show resolved Hide resolved
@dstepanov
Copy link
Contributor

I am not a fan of the naming ByteBody. Maybe we can add HttpBody, HttpBody body() and RequestHttpBody/ResponseHttpBody. There are a few cases right now that can be improved by this hierarchy. In many cases (at least in the server implementation for the request), we set some anonymous body and a few steps later try to analyze its content, so setting the HttpBody with some media type would help eliminate the interface switch. Also, it will help to have the body already analyzed for the reactive cases, etc, avoiding additional checks.
The ResponseHttpBody would be the current ByteBody.

I will look more on Thursday when I'm back.

@yawkat
Copy link
Member Author

yawkat commented May 13, 2024

It is named ByteBody because it specifically deals with bytes. It is independent of HttpRequest.getBody which can also have independent, unserialized beans

@graemerocher
Copy link
Contributor

@yawkat can you rebase?

@yawkat
Copy link
Member Author

yawkat commented May 27, 2024

ive merged 4.5.x into this, we can squash on PR merge

Copy link

@graemerocher graemerocher merged commit 51fe0b4 into 4.5.x May 27, 2024
17 checks passed
@graemerocher graemerocher deleted the bodyng4 branch May 27, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants