Avoid hidding exceptions

[arnaud-lb]

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

When using the RetryExecutor, the exception thrown by the following code is caught, whereas it should not:

$executor = newExecutor($loop, newParser(), newBinaryDumper());
$executor = newRetryExecutor($executor);
$resolver = newResolver('8.8.8.8:53', $executor);
$executor->resolve(null)
// Once the promise is rejected, `done()` throws an exceptions, since there is no `onRejected` callback.// However, the exception is caught by RetryExecutor's `then()`.// This PR fixes this.
->done(function ($result) {
...
});

The PR fixes this by changing some then to done.

[clue]

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

I'm currently in the process of refactoring quite a bit of the dns component, so I'm considering how I'm going to introduce this.

FWIW: These changes only work with the latest Promise API, so we would also have to raise our minimum requirement.

[arnaud-lb]

Thanks.

Currently, catching issues in user code is made very hard because of this, as soon as this then is part of the call path.

Just for my information, what kind of issues could this change introduce ?

FWIW: These changes only work with the latest Promise API, so we would also have to raise our minimum requirement.

What are the rules for raising the requirements against a dependency that's also part of reactphp ? Could this be done in a MINOR (i.e. not PATCH) release of react/dns ?

[clue]

I'm currently in the process of refactoring quite a bit of the dns component, so I'm considering how I'm going to introduce this.
Just for my information, what kind of issues could this change introduce ?

After reconsidering this, I'm leaning towards getting your changes in first. After all you've filed your PR first :-) I'll have to resolve a few merge conflicts because I'm looking into introducing quite a bit of new structure, but that's nothing you have to worry about.

What are the rules for raising the requirements against a dependency that's also part of reactphp ?

This will likely be tackled once it comes to tagging the next release. In this particular case it does not affect the public API, so this will likely end up in a patch release.

Can you update the composer.json?

Otherwise LGTM 👍
ping @WyriHaximus, @jsor

[jsor]

LGTM 👍

[clue]

Can you update the composer.json?

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

Otherwise LGTM 👍

[WyriHaximus]

LGTM 👍

[arnaud-lb]

@clue@WyriHaximus done! Sorry for the delay, I didn't see the update request at first :)

[cboden]

done seems to have caused a test to fail for PHP 5.3

[jsor]

Yes, because react/promise ~2.0|~1.1 is required and it installs ~1.1 on PHP5.3 (~2.0 requires 5.4) which has no done.

[clue]

This will also cause a merge conflict with #35. Is this still relevant?

[arnaud-lb]

@clue Yes it is :)

[arnaud-lb]

@clue: I've rebased the PR against master, to fix the conflicts.

Also, I've updated the composer requirements (react/promise ~2.2), and removed PHP 5.3 from travis' php targets.

[clue]

@arnaud-lb Thanks for filing this PR, keeping it updated and sparking the discussion! 👍

I'm not opposed to getting this in, but I'm currently unsure about how much value this PR (still) provides.

I think the original PR actually addressed a relevant issue of possibly discarding exceptions in the RetryExecutor, but this was actually already addressed as part of #48 in the meantime.

This means that now, the current PR only avoids discarding exceptions in a single location (cache) that never throws exceptions in the first place. To get this PR in, we'd have to sacrifice backwards compatibility with completely outdated versions (which I'm okay with).

As an alternative, I would rather address the cache issue in #81 instead, at which point this update is no longer required in the first place.

What do you think about this? Every way, again thanks for your contribution! 👍

[clue]

@arnaud-lb Thank you for working on this! I'm closing this for now as it hasn't received any input in a while and I believe this has been answered. Please come back with more details if this problem persists and we can reopen this 👍+