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

64KiB Server request limit #412

Closed
HLeithner opened this issue Jul 12, 2021 · 9 comments
Closed

64KiB Server request limit #412

HLeithner opened this issue Jul 12, 2021 · 9 comments
Labels

Comments

@HLeithner
Copy link

It seems the server has a hardcoded 64KiB limit. This isn't actually a problem because it should be possible to override it at least that's what the comment says at
https://github.com/reactphp/http/blob/master/src/Server.php#L180
But if I read the code correctly it's not possible to override this 64 KiB limit.
Looking at https://github.com/reactphp/http/blob/master/src/Server.php#L335 the limit will never exceed the MAXUM_BUFFER_SIZE

Do I missed something?

The only way to overcome this limit is to use the streaming interfaces, if this is right then the comment should be changed.

@WyriHaximus
Copy link
Member

Hmm you might actually be right. Will have a look why this slipped past our tests and how to fixed it in both the code and tests 👍

@HLeithner
Copy link
Author

HLeithner commented Jul 12, 2021

While debugging this it was hard to find out why I got an empty body, may it would sense to set an attribute to the request when the body is truncated?

Also it's hard to response/process to a situation without knowing what happend, because in this case the Middleware just truncates the body and you can't response with a proper code error message.

@clue clue added the question label Jul 25, 2021
@clue
Copy link
Member

clue commented Jul 25, 2021

@HLeithner Thanks for bringing this up. There is indeed a default 64 KiB limit as described in the documentation.

In order to override the above buffering defaults, you can configure the Server explicitly. You can use the LimitConcurrentRequestsMiddleware and RequestBodyBufferMiddleware (see below) to explicitly configure the total number of requests that can be handled at once like this:

$server = new React\Http\Server(
    new React\Http\Middleware\StreamingRequestMiddleware(),
    new React\Http\Middleware\LimitConcurrentRequestsMiddleware(100), // 100 concurrent buffering handlers
    new React\Http\Middleware\RequestBodyBufferMiddleware(2 * 1024 * 1024), // 2 MiB per request
    new React\Http\Middleware\RequestBodyParserMiddleware(),
    $handler
);

https://github.com/reactphp/http#server

I believe this has been answered, so I'm closing this for now. Please come back with more details if this problem persists and we can always reopen this 👍

@clue clue closed this as completed Jul 25, 2021
@HLeithner
Copy link
Author

@clue please look again on the linked code lines, there are 2 bugs in React\Http\Server

  1. in the documentation that you could override RequestBodyBufferMiddleware which is actually not true.
  2. in getMaxRequestSize which has a broken if statement.

@clue
Copy link
Member

clue commented Jul 25, 2021

in the documentation that you could override RequestBodyBufferMiddleware which is actually not true.

Have you checked my above code snippet? This should be covered by the existing test suite. Note that this requires a StreamingRequestMiddleware as given above.

in getMaxRequestSize which has a broken if statement

Can you elaborate? This has 100% code coverage, but if you feel a specific case isn't covered adequately, perhaps add an additional test case? 👍

@HLeithner
Copy link
Author

Ok I missed the Streaming Middleware but it looks like a shady workaround.

I checked the documentation about the post_max_size ini were 64KiB capped is intended. I'm not sure why I'm not allowed to shoot my self in the foot by specifying a bigger ini setting then 64KiB especially when there is a documentation I can do it working around the well tested code.

But yes you are right the function does what it should. thx

@clue
Copy link
Member

clue commented Jul 25, 2021

Ok I missed the Streaming Middleware but it looks like a shady workaround.

I understand where you're coming from, but from experience I would argue that this works good enough. If you feel there's any way to improve this, PRs are very much appreciated! 👍

I checked the documentation about the post_max_size ini were 64KiB capped is intended. I'm not sure why I'm not allowed to shoot my self in the foot by specifying a bigger ini setting then 64KiB especially when there is a documentation I can do it working around the well tested code.

I understand where you're coming from and I think we agree that defaults should be sane. However, the post_max_size is really designed for a non-concurrent execution and for obvious reasons doesn't take into account that multiple requests may occur at the same time. That's why we've decided to cap this by default to allow higher concurrency in favor of bigger uploads by default. See #371, #334, #335, #336, #342 and others.

@HLeithner
Copy link
Author

I see Limiting can be improved on all layers also looking at #411

Thanks for you time and we will see what the future brings, maybe some more dynamic limiting Middleware which can automatically balance between requests and size and maybe park requests or deny them based on server load.

@clue
Copy link
Member

clue commented Jul 25, 2021

Thanks for you time and we will see what the future brings, maybe some more dynamic limiting Middleware which can automatically balance between requests and size and maybe park requests or deny them based on server load.

Agreed, see also #255 if you're looking for ways to contribute to this project 👍

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

No branches or pull requests

3 participants