Request body not available when using Server

[enumag]

Note: I'm completely new to React and async programming. I'm experimenting with both reactphp and amphp to get the gist of what this is all about. So this is my feedback as a complete noob about one of the total WTFs I had with React.

I was trying to setup a very basic HTTP server using an event loop and then integrate https://github.com/webonyx/graphql-php to make a GraphQL server example.

GraphQL takes the query as a JSON in the request body. So I was trying this:

$server = new Server(function (ServerRequestInterface $request) {
    $rawInput = $request->getBody()->getContents();

    $input = json_decode($rawInput, true);
    $query = $input['query'];

    $promise = GraphQL::promiseToExecute(
        new ReactPromiseAdapter(),
        $this->createSchema(),
        $query
    );

	// ...
});

The problem I ran into was that $request->getBody()->getContents() returned an empty string instead of the body.

After some debugging I found the reason:

• Server uses RequestBodyBufferMiddleware to buffer the request body so it should be available.
• However Server also uses RequestBodyParserMiddleware which parses the body (not needed in my case, I need the whole body).
• Unfortunately parsing the body has the side-effect of removing it from the Request object (see https://github.com/ringcentral/psr7/blob/360faaec4b563958b673fb52bbe94e37f14bc686/src/BufferStream.php#L39) which is why getContents() returned an empty string in my handler.
• To make things even worse this does not happen always because the RequestBodyParserMiddleware is only used under certain conditions. This makes the Server quite unreliable.

Once I understood the issue it was of course easy to fix using StreamingServer without the RequestBodyParserMiddleware.

That's all. I'd not call this a bug but it certainly is a complete WTF for a React noob like me. It's up to you if you want to do something about it or not. Doing the same thing in Amphp was very easy in comparison because there was no WTF moment.

---

On a side note React is a complete nightmare to debug for me at the moment since it seems to hide errors by default. I'd like to ask someone more experienced about it but I'm unsure where to ask.

I'm also not quite satisfied with my final code, especially compared to the same thing written with Amphp which most likely means I missed something so I'd like to ask some react expert how to make it cleaner. Not posting it here because it's off-topic.

Where can I reach the React PHP community to ask such questions?

(I was unable to connect to the IRC channel linked on the website. Not to mention that IRC is not exacly a modern platform - it would be much better if there was a gitter, slack, discord or something like that.)

[clue]

@enumag Thank you for filing this issue and the excellent questions!

The problem I ran into was that $request->getBody()->getContents() returned an empty string instead of the body.

That's interesting! I agree that this is a very common use case, so I've just filed #341 to add some documentation and an example for a JSON-based API. I can not reproduce the problem you're seeing in this ticket, so perhaps you can check it out and see if this works for you?

[…] parsing the body has the side-effect of removing it from the Request object (see https://github.com/ringcentral/psr7/blob/360faaec4b563958b673fb52bbe94e37f14bc686/src/BufferStream.php#L39) which is why getContents() returned an empty string in my handler.

Thanks for diving into this and reporting, it sounds like this may in fact be a bug. I haven't tried to reproduce this yet, but perhaps anybody cares to create a PR with a test case for this and with your suggested changes applied?

On a side note React is a complete nightmare to debug for me at the moment since it seems to hide errors by default.

Ouch, but I appreciate the feedback. I'm happy to hear you've got this sorted out besides these initial difficulties nonetheless. I understand that it takes some time to get used to ReactPHP and its execution model. We're very open for input and continuously aiming to improve on this front, so if you happen to find something that you think can be improved, please make sure to let us know! 👍

Where can I reach the React PHP community to ask such questions?

Filing this an issue is what we usually prefer 👍 See also https://reactphp.org/#support for ways to reach out to us or file a meta issue in https://github.com/reactphp/react if you feel there's a way we can do better.

[enumag]

@clue Since you seem to be quite interested in my feedback I went back to find out more.

I can not reproduce the problem you're seeing in this ticket, so perhaps you can check it out and see if this works for you?

The problem was that I was testing the server using this shell command:

curl http://localhost:1337 -d '{"query": "query { echo(message: \"Hello World\") }" }'

As you can see I did not specify the Content-Type header. It seems that something in React added Content-Type: application/x-www-form-urlencoded to my request as a default which is why the RequestBodyParserMiddleware tried to parse it and removed the actual content from BufferStream as a result.

I believe that if I added proper Content-Type: application/json header, it would have worked just fine. It just didn't occur to me to add it while I was just testing React.

In case you want to try it out yourself I pushed my experiments here (checkout the react branch). I reverted it to the errorneous state where the content get's cleared by that middlewere so $rawInput here is an empty string.

Thanks for diving into this and reporting, it sounds like this may in fact be a bug. I haven't tried to reproduce this yet, but perhaps anybody cares to create a PR with a test case for this and with your suggested changes applied?

I'm not really "suggesting" anything at this point. While I do have 10 years of experience with PHP and am an expert in Symfony, Doctrine etc. I'm complete noob here with async programming and React PHP. I just wanted to let you know about the WTF moment I ran into, not really sure what is the proper way of fixing it - leave it as is can also be an option of course.

On a side note React is a complete nightmare to debug for me at the moment since it seems to hide errors by default.

Ouch, but I appreciate the feedback. I'm happy to hear you've got this sorted out besides these initial difficulties nonetheless. I understand that it takes some time to get used to ReactPHP and its execution model. We're very open for input and continuously aiming to improve on this front, so if you happen to find something that you think can be improved, please make sure to let us know! +1

You can actually see my issue with debugging in my experiments repo. In the state the react branch is now (commit https://github.com/enumag/async-experiments/commit/fa99008442ee9c9ff4d93c22672398031a2af40d) the server simply responds with this:

Error 500: Internal Server Error

As you can imagine this is not very helpful to find out the problem.

The running server.php script prints a message

Error: The response callback is expected to resolve with an object implementing Psr\Http\Message\ResponseInterface, but rejected with "Nette\Utils\JsonException" instead.

which is only visible because of this. At first I didn't even have that in so that "Error 500" was literally all I had about the error. No message, no stacktrace, no anything - this is what I mean by react "hiding errors by default". Of course you do have a code like this in the docs but let's be realistic - the readme is quite long and it's not like a noob will thoroughly read it before trying it out - I certainly did not. So if error reporting is not properly setup within the very first example code in the readme, with a note that it's important to have it there, then there is a good chance a noob will accidentally skip it like I did.

Filing this an issue is what we usually prefer +1 See also reactphp.org/#support for ways to reach out to us or file a meta issue in reactphp/react if you feel there's a way we can do better.

I didn't want to fill your issue tracker with questions, but if that's what you prefer, I'm completely fine with it.

Thank you very much for your help!

EDIT: On a side note, code based on react seems to be full of callbacks and then chains so I went to try to add Recoil on top of React right after fixing my React issue. Coroutines are much easier to read in my opinion. Sadly I could not get it to work. Would you have any ideas about that? Of course if you're unfamiliar with Recoil then ignore this.
recoilphp/recoil#164

Just a heads-up: PSR-7 does require to consume the buffer, so the buffer stream implementation is correct. Maybe the parser should create a new stream available for the consumer to consume, if the parser decides to consume the body stream?

[clue]

@clue Since you seem to be quite interested in my feedback I went back to find out more.
[…]
I believe that if I added proper Content-Type: application/json header, it would have worked just fine. It just didn't occur to me to add it while I was just testing React.

@enumag Thank you for reporting back, this is indeed important feedback for us! As a consequence, I've filed #341 to improve documentation for common JSON/XML requests and I hope the next person encountering this problem will be able to find this out right in the updated documentation 👍

curl http://localhost:1337 -d '{"query": "query { echo(message: \"Hello World\") }" }'

As you can see I did not specify the Content-Type header. […]

It looks like curl automatically adds a Content-Type: application/x-www-form-urlencoded request header by default. You can simply pass the -v flag to see its outgoing request headers and incoming response headers.

At first I didn't even have that in so that "Error 500" was literally all I had about the error. No message, no stacktrace, no anything - this is what I mean by react "hiding errors by default". Of course you do have a code like this in the docs but let's be realistic - the readme is quite long and it's not like a noob will thoroughly read it before trying it out - I certainly did not. So if error reporting is not properly setup within the very first example code in the readme, with a note that it's important to have it there, then there is a good chance a noob will accidentally skip it like I did.

Again, thanks for this feedback! As a consequence, I've filed #342 to improve documentation for error reporting as a first step and I hope the next person encountering this will be able to find this out right in the updated documentation 👍 Once this is in, we will keep working on improving default error reporting.

Filing this an issue is what we usually prefer +1 See also reactphp.org/#support for ways to reach out to us or file a meta issue in reactphp/react if you feel there's a way we can do better.

I didn't want to fill your issue tracker with questions, but if that's what you prefer, I'm completely fine with it.

This may or may not be optimal depending on your particular question, but it's something that has worked out quite well for us most of the time. Getting slightly off-topic here, but issues have far better discoverability than more chat-like interactions and as such this helps avoiding having to answer the same question multiple times. As a rule of thumb: If you have a question, somebody else might too.

EDIT: On a side note, code based on react seems to be full of callbacks and then chains so I went to try to add Recoil on top of React right after fixing my React issue. Coroutines are much easier to read in my opinion.

I understand where you're coming from and agree that coroutines appear to solve some of the architectural problems in an async application. It's definitely an interesting concept to discuss, so make sure to also file a separate ticket in this repo if you feel like discussing this further 👍

I'd like to make this ticket actionable and would like to get the next release out. 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 reopen this 👍

[clue]

@CharlotteDunois Just a heads-up: PSR-7 does require to consume the buffer, so the buffer stream implementation is correct. Maybe the parser should create a new stream available for the consumer to consume, if the parser decides to consume the body stream?

I'm not sure I follow what you're suggesting here :) If you feel this is still an issue, please report a dedicated ticket and I'm happy to look into this. 👍

[acasademont]

Hi @clue I just stumbled upon this!

We are in fact using the RequestBodyParserMiddleware and it works great, we're sending an intentional curl request for a form. The problem is that we would also like to keep the whole body but right now is being erased due to the BufferStream implementation. It should be quite easy to fix, you just need to reinject the body contents on the newly created request. I could work on a PR if needed.

The RequestBodyParserMiddlewareTest works because it doesn't use a BufferStream body but a normal string. If that test is changed to use the BufferStream it will fail.

[bramus]

Running into the same issue here and was (still am, actually) really scratching my head.

If I summarise this discussion then problem is that the middleware mentioned can't properly handle scenarios where a Content-Type header is missing.

I've tried changing my implementation to using React\Http\StreamingServer as suggested but to no avail — it even got worse then: the $request now reported the proper size (Woohoow! 🎉) but the body itself remained empty (Huh? 🤔). See examples below for details.

Not too sure how I can solve this I must say …

To be complete, here are the versions I'm running:

        "react/event-loop": "^1.1",
        "react/http": "^0.8.6",

Example using React\Http\Server

Code:

$server = new React\Http\Server(function ($request) {
        var_dump($request->getBody()->getSize());
        var_dump((string) $request->getBody());
        var_dump($request->getParsedBody());
});

Request with header

curl -X POST -d '{"foo": "bar"}' -H 'Content-Type: application/json'  http://localhost:8080/

int(14)
string(14) "{"foo": "bar"}"
NULL

Note: From its name I expected getParsedBody to have actually parsed the JSON (because of the header being present), yet now it returns NULL

Request without header

curl -X POST -d '{"foo": "bar"}' http://localhost:8080/

int(0)
string(0) ""
array(1) {
  ["{"foo":_"bar"}"]=>
  string(0) ""
}

Note: That output for getParsedBody sure looks weird …

Example using React\Http\StreamingServer

Code:

$server = new React\Http\StreamingServer(function ($request) {
        var_dump($request->getBody()->getSize());
        var_dump((string) $request->getBody());
        var_dump($request->getParsedBody());
});

Request with header

curl -X POST -d '{"foo": "bar"}' -H 'Content-Type: application/json'  http://localhost:8080/

int(14)
string(0) ""
NULL

Request without header

curl -X POST -d '{"foo": "bar"}' http://localhost:8080/

int(14)
string(0) ""
NULL

Suggestions

As a user, if I may suggest:

1. The Middleware / Server should work fine in scenarios where no headers are sent (because you don't always control the client that is making the requests). My suggestion would be to leave the message body untouched.
2. A call to $request->getBody() should always return the “raw” body (now it appears to get eaten by the middleware even though its reported size is bigger than 0)

If I understand things correctly I think suggestion 2 will be an automatic result of fixing 1.

If I summarise this discussion then problem is that the middleware mentioned can't properly handle scenarios where a Content-Type header is missing.

The RequestBodyParser only handles Content-Type header x-www-form-urlencoded and multipart. If there's no Content-Type header or the value isn't one of the two mentioned, then the middleware does not nothing and invokes the next middleware.

However the issue comes from curl as it automatically adds Content-Type: application/x-www-form-urlencoded to the request.

>curl -v -X POST -d '{"foo": "bar"}' http://localhost:80/
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 80 (#0)
> POST / HTTP/1.1
> Host: localhost
> User-Agent: curl/7.55.1
> Accept: */*
> Content-Length: 6
> Content-Type: application/x-www-form-urlencoded
>
* upload completely sent off: 6 out of 6 bytes
< HTTP/1.1 200 OK
< Date: Thu, 26 Mar 2020 10:32:54 GMT
< Server: Apache/2.4.18 (Win32) OpenSSL/1.0.2e PHP/7.0.8
< Content-Length: 0
< Content-Type: text/html; charset=UTF-8
<
* Connection #0 to host localhost left intact

I've tried changing my implementation to using React\Http\StreamingServer as suggested but to no avail — it even got worse then: the $request now reported the proper size (Woohoow! 🎉) but the body itself remained empty (Huh? 🤔). See examples below for details.

That's because the request body hasn't been received yet. As the name suggests it's a streaming server - the request body gets streamed to you, so it would be your job to buffer the request body. The Server class buffers the request body for you and holds back the request until the request body has been fully received (or an error occurs).

[bramus]

However the issue comes from curl as it automatically adds Content-Type: application/x-www-form-urlencoded to the request.

Damn I feel stupid right now. Was already digging into the RequestBodyParser and indeed saw the conditionals there to only parse things in case x-www-form-urlencoded was set, yet still couldn't pinpoint why things went wrong.

Thanks for pointing out that curl auto-injected the header, @CharlotteDunois! All is clear now :)