Avoid hidding exceptions thrown in HttpClient\Request error handlers

[arnaud-lb]

then() catches exceptions, which is not desirable when its result is not returned.

The following code throws an exception in the 'error' event handler. However, the exception is caught because the event is fired in a then() callback.

This PR fixes this by changing a then to a done.

$requestData = newRequestData('GET', 'http://www.example.com');
$request = newRequest($this->connector, $requestData);
$request->on('error', function () {
thrownew \Exception();
});
$request->end();

Actual use case:

$requestData = newRequestData('GET', 'http://www.example.com');
$request = newRequest($this->connector, $requestData);
$deferred = newDeferred();
$request->on('error', function ($err) use ($deferred) {
$deferred->reject($err);
});
$deferred->done(); // Throws an exception if the deferred is rejected.// Without the PR, the exception is caught.$request->end();

[clue]

Thanks for spotting! These could certainly introduce some subtle and hard to trace issues.

Afaict you're trying to throw an Exception in the data (or error) event handler? I'm not quite sure this has been tested extensively. Also, I'm not quite sure how this should be handled ideally.

Can you provide a simple gist that exhibits this?

[arnaud-lb]

Hi @clue,

Afaict you're trying to throwanExceptionin thedata(orerror`) event handler? I'm not quite sure this has been tested extensively. Also, I'm not quite sure how this should be handled ideally.

I guess the request/response shouldn't be considered to be in a consistent state after that :)

But at least, the exception should be allowed to unwind and eventually trigger a visible error if not caught by user code.

I'm not quite sure this has been tested extensively

I'm guilty, I didn't planned for that when I wrote reactphp/http-client :)

Can you provide a simple gist that exhibits this?

Sure ! Here it is: https://gist.github.com/arnaud-lb/71cf44ab2564b810b463

As you can see, I'm not throwing an exception myself. (And at first, one wouldn't think that the exception is thrown in the error event handler; but it is.)

However, I'm expecting to at least see exceptions that are thrown.

The problem disappears after merging both reactphp/dns#26 and #31.

Thanks!

[WyriHaximus]

Going to review and test this tonight and get back to it here, thanks for spotting 👍

[WyriHaximus]

LGTM 👍 , @clue do you want to check, merge and tag reactphp/dns#26 first before signing this one off or should I go ahead and merge and tag this one first?

[clue]

Thanks for the background @arnaud-lb!

LGTM 👍

@WyriHaximus both PRs LGTM, so feel free to review the other one and push either one forward 👍

[clue]

Ping @arnaud-lb, can you update this to require react/promise: ~2.2?

Otherwise LGTM 👍

[jsor]

LGTM 👍