Skip to content
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.

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

Error handler might still bubble up #42

Closed
kelunik opened this issue Jan 20, 2017 · 7 comments
Closed

Error handler might still bubble up #42

kelunik opened this issue Jan 20, 2017 · 7 comments

Comments

@kelunik
Copy link
Member

kelunik commented Jan 20, 2017

Exceptions forwarded to ErrorHandler might still bubble up if an error handler is set with set_error_handler and that one throws. Is there any reasonable way to prevent that except for calling exit; directly?

@bwoebi
Copy link
Member

bwoebi commented Jan 20, 2017

If there is an error happening. Well, if someone is writing code which emits errors …

@kelunik
Copy link
Member Author

kelunik commented Jan 20, 2017

@bwoebi What's your point?

@trowski
Copy link
Contributor

trowski commented Jan 20, 2017

Just to be clear, you're suggesting that error handlers such as the one below will still cause errors to bubble up from promise callbacks.

set_error_handler(function ($errno, $errstr) {
    throw new \ErrorException($errstr, 0, $errno);
});

I don't think theres any way to prevent this other than calling exit. I looked at various solutions including setting a different error handler, but nothing made sense or just ended up going in circles.

It might be best to just wrap the current try/catch block with the following:

try {
    // Current try/catch block invoking error handler.
} catch (\Throwable $exception) {
    // Should we write to STDERR or just exit?
    if (\is_resource(STDERR)) { // STDERR could have been closed, so to avoid more error madness...
        \fwrite(STDERR, "It's not nice to throw things!\n");
    }
    exit(1);
} catch (\Exception $exception) {
    // Since we're still PHP 5.x compatible.
    if (\is_resource(STDERR)) {
        \fwrite(STDERR, "It's not nice to throw things!\n");
    }
    exit(1);
}

@kelunik
Copy link
Member Author

kelunik commented Jan 21, 2017

Bluebird does in fact just the same, I'm just not sure about the exit code. https://github.com/petkaantonov/bluebird/blob/master/src/async.js#L49-L57

@kelunik
Copy link
Member Author

kelunik commented Jan 21, 2017

We should probably just use 255, just as php -r 'throw new Exception;'; echo $? does.

kelunik added a commit that referenced this issue Jan 21, 2017
kelunik added a commit that referenced this issue Jan 21, 2017
kelunik added a commit that referenced this issue Jan 22, 2017
@WyriHaximus
Copy link
Member

While I'm still strongly against using an exit in our error handlers, #43 solves it decently assuming the behavior will also be documented in the readme that implementers should avoid this edge case at all cost 👍

@kelunik
Copy link
Member Author

kelunik commented Jan 22, 2017

@WyriHaximus I've added a section to the README in the PR.

I don't see a way to avoid exit while at the same time guaranteeing that the exception does not bubble up to the place where $deferred->resolve() etc. are called.

@bwoebi bwoebi closed this as completed in #43 Feb 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants