[ReactPHP] Handle uncaught errors by sending their error message as response back #9538
Conversation
frameworks/PHP/reactphp/app.php
Outdated
| default => new Response(404, [], 'Error 404'), | ||
| }; | ||
| }))->catch( | ||
| static fn (Throwable $error): PromiseInterface => resolve(Response::plaintext($error->getMessage())->withStatus(200)), // This should be a 500, but with a 200 the benchmarking tooling considers the framework alive |
There was a problem hiding this comment.
It's OK to send a 500 response to the tests.
The problem is in React or the code. The app is blocked again when send a 500.
Also end stuck when send a 400 or 404.
I said it to fix the root of the problem.
You can check it with the plain PHP files, and send a 500 error, the tests will continue normally and the tests fail normally. Never show reactphp: ERROR: Framework is not accepting requests from client machine.
There was a problem hiding this comment.
Also update your local repo, because my PR is included now, and the db tests are disabled.
So never will arrive to this catch().
So this PR need to include again the tests.
There was a problem hiding this comment.
But if any test fail, never will be merged !!
There was a problem hiding this comment.
It's OK to send a 500 response to the tests.
Sure, I don't doubt that. But it's not during the liveness check. As per https://explainshell.com/explain?cmd=curl+--fail
The problem is in React or the code. The app is blocked again when send a 500. Also end stuck when send a 400 or 404.
Actually, it is not, you made a very conscious decision not to allow any status of 400 and high to be considered ready for benchmarking with #3709. So the problem is in the design that checks an arbitrary endpoint to see if the entire application is ready rather than the one with the least complexity and dependencies on other services that could cause issues.
I said it to fix the root of the problem.
I can do that by creating a PR that drops the --fail flag, but you've been very strongly opposed to any changes to the toolset.
You can check it with the plain PHP files, and send a 500 error, the tests will continue normally and the tests fail normally. Never show
reactphp: ERROR: Framework is not accepting requests from client machine.
Again tests I don't doubt that, but for the reasons mentioned about you've purposely blocking 500's
Also update your local repo, because my PR is included now, and the db tests are disabled. So never will arrive to this catch().
So this PR need to include again the tests.
Just did that.
But if any test fail, never will be merged !!
Yes, you've made that very clear on several occasions now.
There was a problem hiding this comment.
@WyriHaximus good catch with the --fail in #3709
I'm strongly opposed to any unilateral change in the toolset, like you did.
Your change to use "plaintext" will fix the problem with React, but will break hundreds of frameworks variants.
When a framework variant test for another db, only create the db endopints, so it'll fail to check the "plaintext".
Also a change in the toolset, need to test all the frameworks with the Github actions, that is a very long time, and always will fail with some frameworks.
Please create a issue for this change, and I'll support it.
So all the devs can discus it and the people from Techempower will decide what to do.
I think that remove that --fail don't need to much discussion, but also we need to check if it's breaking a lot of frameworks.
PD: I'm not affiliated to Techempower in any way
There was a problem hiding this comment.
@WyriHaximus good catch with the
--failin #3709I'm strongly opposed to any unilateral change in the toolset, like you did. Your change to use "plaintext" will fix the problem with React, but will break hundreds of frameworks variants. When a framework variant test for another db, only create the db endopints, so it'll fail to check the "plaintext". Also a change in the toolset, need to test all the frameworks with the Github actions, that is a very long time, and always will fail with some frameworks.
Please create a issue for this change, and I'll support it. So all the devs can discus it and the people from Techempower will decide what to do. I think that remove that
--faildon't need to much discussion, but also we need to check if it's breaking a lot of frameworks.
Will do, and as you mention that change will affect every framework in here. So will think about it for a while before filing it.
PD: I'm not affiliated to Techempower in any way
Honestly, that doesn't matter to me. You're responding to all my PR's and comments with suggestions or context. Techempower affiliation isn't required there for me. In fact, I even appreciate it that you are spending this much time help out on this project <3.
There was a problem hiding this comment.
If you want I can create the issue.
There was a problem hiding this comment.
There was a problem hiding this comment.
I know more this bench, and I understand you.
But it'll be easier for me !!!
There was a problem hiding this comment.
Both are fine with me, if you do please ping me and I'll put my thoughts in there 👍 .
WyriHaximus
force-pushed
the
reactphp-handle-uncaught-errors
branch
from
3014f1f to
4541529
Compare
|
Where does this one stand? Ready to merge or waiting on changes? |
|
Ready to be merged!! |
3 participants
No description provided.