Call of done() is absent

[redbarmaley]

Good day.

Here in \React\Http\StreamingServer::handleRequest (StreamingServer.php:274) call of done() is absent. This may lead to ignoring the error.

[clue]

@RedKlaus I'm not sure I follow what problem you're seeing with the following code:

http/src/StreamingServer.php

Line 274 in b29ab96

if (!$response instanceof ResponseInterface) {

Can you be more specific in what error is being ignored and/or provide a small gist to reproduce this?

[redbarmaley]

Sorry. Line #299. https://github.com/reactphp/http/blob/master/src/StreamingServer.php#L299

[clue]

Can you be more specific in what error is being ignored and/or provide a small gist to reproduce this?

[redbarmaley]

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]

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]

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');
});

[clue]

@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]

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]

reactphp/promise docs says:

Either return your promise, or call done() on it.

This is a "golden rule" and I think it is a good one.

[WyriHaximus]

reactphp/promise docs says:

Either return your promise, or call done() on it.

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:

$server->on('error', CallableThrowableLogger::create($logger));

$logger is a PSR-3 logger instance, and CallableThrowableLogger is from wyrihaximus/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]

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]

@WyriHaximus,
let me give you another example:

\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.

[WyriHaximus]

@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:

\React\Promise\resolve()->then(function () {
        throw new Exception('some error');
})->then(null, function (Exception $e) {
        var_dump($e);
});

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.

[kyptov]

@WyriHaximus thanks. Now I understand why not to done.

[redbarmaley]

@WyriHaximus, ok) I take my words back. I'm sorry.