-
Notifications
You must be signed in to change notification settings - Fork 368
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
Can't read buffered request bodies? #529
Comments
Check latest react/http release. Remember reading something that might be related. |
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. |
@Firehed how would you then suggest we could do this. I also thought react/http 1.2 would fix this |
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 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:
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 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 |
Master allows 1.3 now. Does that help? |
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
inServerRequest->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 theStreamInterface
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
/StreamInterface
s 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.The text was updated successfully, but these errors were encountered: