Skip to content

Support for configuring max. concurrent requests and request body buffer - #505

Merged
andig merged 3 commits into
php-pm:masterfrom
klemenb:master
Jul 27, 2020
Merged

Support for configuring max. concurrent requests and request body buffer#505
andig merged 3 commits into
php-pm:masterfrom
klemenb:master

Conversation

@klemenb

@klemenb klemenb commented Jul 24, 2020

Copy link
Copy Markdown
Contributor

This PR adds configuration options for setting the parameters of LimitConcurrentRequestsMiddleware and RequestBodyBufferMiddleware middlewares in ReactPHP server.

This allows us to handle request bodies larger than 64K after the 2.1.0 release that includes reactphp/http#371.

This PR is currently incomplete due to how we handle default values and I cannot really decide on the options below:

  1. Keep the default ReactPHP server behavior by default and manually configure these two middlewares only when both of the new configuration options are present.

  2. Remove the default behavior (HttpServer initialization without these middlewares) and configure default values in PHP-PM. We could set them to 1024 and 64K. This is probably the most clear way of configuring them from the PHP-PM perspective, but it does stop the default ReactPHP calculation from working (the way it calculates these values is described here - https://github.com/reactphp/http#server).

  3. Keep the current PR code and only add default values in HttpServer initialization when only one of the two options is present. Maybe just use 1024 and 64K again for the default values, but only apply them to the undefined configuration parameter. E.g.:

$httpServer = new HttpServer(
    $this->loop,
    new StreamingRequestMiddleware(),
    new LimitConcurrentRequestsMiddleware($this->config['limit-concurrent-requests'] ?? 1024),
    new RequestBodyBufferMiddleware($this->config['request-body-buffer'] ?? HttpServer::MAXIMUM_BUFFER_SIZE),
    new RequestBodyParserMiddleware(),
    [$this, 'onRequest']
);

Also, the naming of these configuration options and the descriptions can be changed. Any suggestions are welcome.

@andig

andig commented Jul 25, 2020

Copy link
Copy Markdown
Contributor

Looking good to me- happy to merge if you are fine with this PR?

@klemenb

klemenb commented Jul 27, 2020

Copy link
Copy Markdown
Contributor Author

We can discuss further on the mentioned 3 options regarding the default values, if you'd like. I went ahead and commited changes to go with the option (3) - add default value to the other parameter if only one is set.

If this is OK with you, you can go ahead with the merge.

@andig
andig merged commit df654ba into php-pm:master Jul 27, 2020
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