Can't read buffered request bodies?

[Firehed]

Relating to #523 (and others) - following those changes, I'm now seeing larger requests being buffered. On its own that's fine, but when larger requests are buffered, I'm now observing that when I try to read the request body, I always get an empty string.

That's caused by receiving React\Http\Io\HttpBodyStream in ServerRequest->getBody(), which has an out-of-spec __toString() implementation. Given that class is marked a @internal, I'm assuming that my actual request handler is receiving that in error.

In the meantime, I can make the buffer size high enough to bypass the problem, but that's not a real solution.

My bridge does nothing special - basic URL-based routing, and the request handler is (effectively) doing json_decode((string)$request->getBody()) which according to the StreamInterface spec should work.

Can you help establish if this is a bug (either here or in react/http), configuration error, or if there's something that needs to be performed on all ServerRequestInterface/StreamInterfaces to establish if there's buffering occurring and ensure that everything is read before the request is handled? This behavior doesn't seem to be noted in the PHP-PM documentation anywhere.

[andig]

Check latest react/http release. Remember reading something that might be related.

[Firehed]

I tried updating to react/http 1.2.0 which just arrived a couple hours ago, but doing so didn't appear to improve things.

After digging through that library's docs a bit more I did find this section which explains how to read the contents of the buffer. However that's done at the low-level server event loop so implementing the same promise-based implementation to read the buffered data doesn't seem possible in the user-space of PHP-PM.

[acasademont]

@Firehed how would you then suggest we could do this. I also thought react/http 1.2 would fix this

[Firehed]

Sorry about the long delay here. I had this recently come up again in production, after previously working around it by raising the limit (a lot).

I did some renewed digging, and AFAICT the second parameter to RequestBodyBufferMiddleware is being used incorrectly (or at least in a way that's inconsistent with the setting's documentation). It seems to indicate the functional equivalent of the maximum request body size allowed (and defaults to post_max_size), and not "buffer bodies over this size"/"stream out the body in chunks of this size", which is what the php-pm (and react!) docs seem to suggest.

Given that, I'm not really sure what the best course of action here is. I think it'd be best to tweak the server process middleware setup and the corresponding config flag names, but most changes here would probably be (minor) breaking changes:

• request-body-buffer changes into max-body-size: int|string (or post-max-size to match the ini name)
• The server middleware setup in ProcessSlave successively adds middlewares based on the config, rather than an all-or-nothing approach (e.g. limit-concurrent-requests is independent of buffering)
• Do something that triggers 413 errors if the value is exceeded (see the old middleware's behavior here)
• Apply something like the linked middleware to rebuild the chunked request

In effect, replace the current setup (in ProcessSlave::doConnect()):

if (limit-concurrent-requests || request-body-buffer) {
  $server = new HttpServer(loop, StreamingRequestMiddleware, LimitConcurrentRequestsMiddleware, RequestBodyBufferMiddleware, RequestBodyParserMiddleware, onRequest)
} else {
  $server = new HttpServer(loop, onRequest)
}

with something like this:

$middlewares = [];
if ($limitConcurrentRequests) {
  $middlewares[] = new LimitConcurrentRequestsMiddleware(...);
}
$middlewares[] = new StreamingRequestMiddleware();
$middlewares[] = function (RequestInterface $request) {
  $body = $request->getBody();
  // assert body is streaming, readablestream
  return new \React\Promise\Promise(function ($resolve, $reject) use ($body) {
    $bodyBuffer = '';
    $body->on('data', function ($data) use (&$bodyBuffer) {
      $bodyBuffer .= $data;
    });
    // on end, convert into request and resolve the promise like linked example 
  });
};
$middlewares[] = new RequestBodyParserMiddleware(); 

$server = new HttpServer($loop, ...$middlewares, [$this, 'onRequest']);

I had moderate success with the above with both of the existing conditional values disabled. In effect, it replaces the poorly-named RequestBodyBufferMiddleware with an "unbuffer middleware".

In the short-term, maybe just a docs change? It's a really strange interaction and I'm having difficulty following all of the implications. IMO it's more of an upstream issue with React's internals/docs

[andig]

I also thought react/http 1.2 would fix this

Master allows 1.3 now. Does that help?