Skip to content

Avoid hidding exceptions #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
arnaud-lb wants to merge 1 commit into from
Closed

Avoid hidding exceptions #26

arnaud-lb wants to merge 1 commit into from

Conversation

@arnaud-lb
Copy link
Contributor

@arnaud-lb arnaud-lb commented May 27, 2015

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 = new Executor($loop, new Parser(), new BinaryDumper());
$executor = new RetryExecutor($executor);
$resolver = new Resolver('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
Copy link
Member

clue commented Jun 7, 2015

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
Copy link
Contributor Author

arnaud-lb commented Jun 8, 2015

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 ?

@arnaud-lb arnaud-lb mentioned this pull request Jun 8, 2015
Merged
@clue
Copy link
Member

clue commented Sep 7, 2015

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
Copy link
Member

jsor commented Sep 8, 2015

LGTM 👍

@clue
Copy link
Member

clue commented Sep 23, 2015

Can you update the composer.json?

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

Otherwise LGTM 👍

@WyriHaximus
Copy link
Member

WyriHaximus commented Sep 24, 2015

LGTM 👍

@arnaud-lb
Copy link
Contributor Author

arnaud-lb commented Dec 31, 2015

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

@cboden
Copy link
Member

cboden commented Jan 19, 2016

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

@jsor
Copy link
Member

jsor commented Jan 19, 2016

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
Copy link
Member

clue commented May 20, 2016

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

@arnaud-lb
Copy link
Contributor Author

arnaud-lb commented May 23, 2016

@clue Yes it is :)

@arnaud-lb arnaud-lb force-pushed the unhide-exceptions branch 2 times, most recently from 02f8163 to de3b479 Compare October 4, 2016 15:23
@arnaud-lb
Copy link
Contributor Author

arnaud-lb commented Oct 4, 2016

@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 clue added the bug label Nov 16, 2016
@arnaud-lb arnaud-lb mentioned this pull request Jan 31, 2017
Closed
@clue
Copy link
Member

clue commented Sep 12, 2017

@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 clue added maintenance and removed bug labels Sep 12, 2017
@clue
Copy link
Member

clue commented Jun 29, 2018

@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 👍+

@clue clue closed this Jun 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

maintenance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@arnaud-lb @clue @jsor @WyriHaximus @cboden