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

MaxContentLength support (streaming) #3319

Merged
merged 10 commits into from
Nov 23, 2023
Merged

Conversation

kciesielski
Copy link
Member

@kciesielski kciesielski commented Nov 15, 2023

Started as a solution to #3056 for Netty, but addresses more backends in general.
This PR introduces a possibility to add a MaxContentLength attribute to any endpoint. When set, decoding RequestBody in supported backends prevents loading too much into memory, and fails if the limit is exceeded.
As a result, DefaultExceptionHandler returns HTTP 413 Payload Too Large.

Streaming only
This is only a part of the full feature. The PR covers streaming support for streaming request body, excluding:

  • Armeria streams
  • Vertx streams
  • zio1 streams

In a follow-up PR, I'll add:

  • Support for non-streaming body (direct byte arrays, files)
  • Documentation
  • API extension to allow writing endpoint.maxContentLength(x) instead of setting an attribute.

@kciesielski kciesielski changed the title [WIP] MaxContentLength support [WIP] MaxContentLength support (streaming) Nov 22, 2023
@kciesielski kciesielski force-pushed the 3056-max-content-length branch from ffb2afd to e61e396 Compare November 22, 2023 22:22
@kciesielski kciesielski changed the title [WIP] MaxContentLength support (streaming) MaxContentLength support (streaming) Nov 23, 2023
@kciesielski kciesielski marked this pull request as ready for review November 23, 2023 08:25
@kciesielski kciesielski requested a review from adamw November 23, 2023 08:25
@kciesielski
Copy link
Member Author

@Mergifyio update

Copy link
Contributor

mergify bot commented Nov 23, 2023

update

✅ Branch has been successfully updated

}

private def nettyRequestBytes(serverRequest: ServerRequest): F[Array[Byte]] = serverRequest.underlying match {
case req: FullHttpRequest => monad.delay(ByteBufUtil.getBytes(req.content()))
case _: StreamedHttpRequest => toStream(serverRequest).compile.to(Chunk).map(_.toArray[Byte])
case _: StreamedHttpRequest => toStream(serverRequest, maxBytes = None).compile.to(Chunk).map(_.toArray[Byte]) // TODO
Copy link
Member

Choose a reason for hiding this comment

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

TODO ?:)

@adamw
Copy link
Member

adamw commented Nov 23, 2023

It seems there's a half-done maxContentLegth implementation for netty, in NettyServerHandler, plus a test in ServerBasicTests, which is only enabled for netty, though. Would be good to clean this up in the next PR as well :)

}

private def nettyRequestBytes(serverRequest: ServerRequest): RIO[Env, Array[Byte]] = serverRequest.underlying match {
case req: FullHttpRequest => ZIO.succeed(ByteBufUtil.getBytes(req.content()))
case _: StreamedHttpRequest => toStream(serverRequest).run(ZSink.collectAll[Byte]).map(_.toArray)
case _: StreamedHttpRequest => toStream(serverRequest, maxBytes = None).run(ZSink.collectAll[Byte]).map(_.toArray) // TODO
Copy link
Member

Choose a reason for hiding this comment

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

that's for the next PRs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, coming soon :) #3337

Copy link
Member

@adamw adamw left a comment

Choose a reason for hiding this comment

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

Nice (& tedious) work, thanks :)

@kciesielski
Copy link
Member Author

It seems there's a half-done maxContentLegth implementation for netty, in NettyServerHandler, plus a test in ServerBasicTests, which is only enabled for netty, though. Would be good to clean this up in the next PR as well :)

That current netty-specific maxContentLength support is actually an interesting problem to discuss. It's

  1. global, not per endpoint
  2. affects both responses and requests
  3. does not work for streaming

If we remove it entirely in favor of the new per-endpoint solution, we will lose features of 1) global scope 2) limiting response body.
If we keep it, it may be confusing to understand why there are 2 ways of setting max content length for Netty. Maybe we should limit it to responses only? Or deprecate it and observe who complains that they actually wanted 1 and 2, and not a per-endpoint request limiting?

@kciesielski kciesielski merged commit 961b93a into master Nov 23, 2023
15 checks passed
@mergify mergify bot deleted the 3056-max-content-length branch November 23, 2023 09:44
@adamw
Copy link
Member

adamw commented Nov 23, 2023

Ideally, we want both: a global setting in ServerOptions, with a per-endpoint override (including disabling the global value). Though you can always map over all server endpoints and add the attribute, so maybe that's not needed 🤔

But we should definitely have one solution, so let's keep the "new" one only. I can't see limiting response size as a useful feature (you can do it in the business logic anyway)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants