You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I'm not sure I follow what problem we're discussing here, my take away is... well, if we break the code, then it's broken. Right now, I'm still not seeing anything that is broken.
If you can provide a test case and/or gist to reproduce a problem, then I'm happy to look into this and help working out a solution. 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 👍
On top of this, we're working on logging unhandled promise rejections as part of the promise library via reactphp/promise#87 in the future. Once this is in upstream, any unhandled promise rejection should at least log a warning without any changes downstream in this component.
When we open this page, we will got 504 error and there is no any error in any log.
$server = new Http\Server(function (ServerRequestInterface $request) {
return new \stdClass();
});
$server->on('error', function ($e) {
throw new \RuntimeException('Unhandled error');
});
@redklaus Thanks for providing a short gist to reproduce this. In fact, you're not the first to run into this known limitation (e.g. clue/reactphp-redis#74) which is why we've started working on this a while ago.
As a quick solution, throwing from any event handler is not allowed, so you can simply change your code to manually wrap this in a try-catch block and handle it accordingly for your use case. After all, what is supposed to happen with this exception in the first place?
As a longer-term solution, this will be addressed upstream via reactphp/promise#87 and will likely report some form of warning in the future.
If you feel there's a better way to handle this, make sure to let us know and I'm happy to look into this 👍
This is a "golden rule" and I think it is a good one.
Yes in most cases this is true, but in this specific case we're handling the error in the final then. What I usually do, and also suggest @redklaus looks into, is logging the error in the on error handler like this:
\React\Promise\resolve()->then(function () {
throw new Exception('some error');
}, function (Exception $e) {
var_dump($e);
});
Exception will be silenced. handleResponse method has 100+ lines of code and for me is too much to be sure its a bulletproof piece of code. It is looking like you asking me to trust this code and to trust every future changes, but sorry I can`t.
But if you replace then with done in Line 272 like this :
\React\Promise\resolve()->done(function () {
throw new Exception('some error');
}, function (Exception $e) {
var_dump($e);
});
Exception will be shown, golden rule is followed and no additional calls for every response is need.
The thing is we still support react/promise 1 in this package (aside with 2) and 1 doesn't have support for done in it. Hence we don't use done in there, we can fix this in two ways, either file a PR here or get @jsor involved and see if we can fix this in react/promise.
Activity
clue commentedon Apr 10, 2019
@redklaus I'm not sure I follow what problem you're seeing with the following code:
http/src/StreamingServer.php
Line 274 in b29ab96
Can you be more specific in what error is being ignored and/or provide a small gist to reproduce this?
redbarmaley commentedon Apr 10, 2019
Sorry. Line #299. https://github.com/reactphp/http/blob/master/src/StreamingServer.php#L299
clue commentedon Apr 10, 2019
Can you be more specific in what error is being ignored and/or provide a small gist to reproduce this?
redbarmaley commentedon Apr 10, 2019
If we add
throw new \RuntimeException();in line #274 (https://github.com/reactphp/http/blob/master/src/StreamingServer.php#L274), this error will be ignored.clue commentedon Apr 10, 2019
I'm not sure I follow what problem we're discussing here, my take away is... well, if we break the code, then it's broken. Right now, I'm still not seeing anything that is broken.
If you can provide a test case and/or gist to reproduce a problem, then I'm happy to look into this and help working out a solution. 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 👍
On top of this, we're working on logging unhandled promise rejections as part of the promise library via reactphp/promise#87 in the future. Once this is in upstream, any unhandled promise rejection should at least log a warning without any changes downstream in this component.
redbarmaley commentedon Apr 10, 2019
When we open this page, we will got 504 error and there is no any error in any log.
clue commentedon Apr 10, 2019
@redklaus Thanks for providing a short gist to reproduce this. In fact, you're not the first to run into this known limitation (e.g. clue/reactphp-redis#74) which is why we've started working on this a while ago.
As a quick solution, throwing from any event handler is not allowed, so you can simply change your code to manually wrap this in a try-catch block and handle it accordingly for your use case. After all, what is supposed to happen with this exception in the first place?
As a longer-term solution, this will be addressed upstream via reactphp/promise#87 and will likely report some form of warning in the future.
If you feel there's a better way to handle this, make sure to let us know and I'm happy to look into this 👍
redbarmaley commentedon Apr 10, 2019
The simplest way is call
done()in https://github.com/reactphp/http/blob/master/src/StreamingServer.php#L299. This fix will show any error in any event handler.kyptov commentedon Apr 10, 2019
reactphp/promise docs says:
This is a "golden rule" and I think it is a good one.
WyriHaximus commentedon Apr 10, 2019
Yes in most cases this is true, but in this specific case we're handling the error in the final then. What I usually do, and also suggest @redklaus looks into, is logging the error in the on error handler like this:
$loggeris a PSR-3 logger instance, andCallableThrowableLoggeris fromwyrihaximus/psr-3-callable-throwable-logger^ That will push the error onto a logger and you can ship it off where you like it like loggly or your STDOUT for local logging
redbarmaley commentedon Apr 11, 2019
Thank you for your advice, but I don't trust your code anymore)
I no need to see errors thrown by me. I need to see errors, that occure by unknown reasons. Because shit happens with all kind of programmers)
kyptov commentedon Apr 11, 2019
@WyriHaximus,
let me give you another example:
Exception will be silenced.
handleResponsemethod has 100+ lines of code and for me is too much to be sure its a bulletproof piece of code. It is looking like you asking me to trust this code and to trust every future changes, but sorry I can`t.But if you replace
thenwithdonein Line 272 like this :Exception will be shown, golden rule is followed and no additional calls for every response is need.
WyriHaximus commentedon Apr 11, 2019
@kyptov
Thank you for those examples, it gave me the click the problem I needed. We could simply fix that in this package using the following code:
The thing is we still support
react/promise1 in this package (aside with 2) and 1 doesn't have support fordonein it. Hence we don't use done in there, we can fix this in two ways, either file a PR here or get @jsor involved and see if we can fix this inreact/promise.kyptov commentedon Apr 17, 2019
@WyriHaximus thanks. Now I understand why not to
done.redbarmaley commentedon Apr 17, 2019
@WyriHaximus, ok) I take my words back. I'm sorry.