Skip to content

[Observer] Fatal error support #6114

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

Closed
wants to merge 5 commits into from
Closed

Conversation

SammyK
Copy link
Contributor

@SammyK SammyK commented Sep 11, 2020

This is another follow up PR to #5857.

This fixes an issue where fatal errors would not call the observer end handlers. And also adds a few missing tests.

Copy link
Contributor

@morrisonlevi morrisonlevi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nikic
Copy link
Member

nikic commented Sep 12, 2020

Does this work with ext/soap or other exts that throw fatal errors and then catch the bailout?

@beberlei
Copy link
Contributor

@nikic Yes it should be, zend_error_impl is called regardless of being "in soap" or not for errors/warnings/... when they occur, this happens outside the php_error_cb which can be replaced, its placed near the error notify callback code, which was also built to always trigger.

@nikic
Copy link
Member

nikic commented Sep 14, 2020

@beberlei My concern is the other way around: The fatal error will be caught by soap and execution will continue, exiting some frames that have already been exited.

@SammyK
Copy link
Contributor Author

SammyK commented Sep 16, 2020

@nikic Very good point. I can confirm that the end handlers get fired twice in the case of ext/soap fatal errors. I'll see if I can come up with an elegant solution to handle the edge cases that extensions like ext/soap present. 🤔

Tangent: Ideally this PR would address all zend_bailouts at a general level, but I'm pretty sure addressing all the edge cases is impossible. I'm wondering if php-src is at the point now where we can seriously discuss a plan to kill zend_bailout with fire? 🔥

@SammyK SammyK force-pushed the observer/fatal_errors branch from 9241436 to 687b7c1 Compare September 25, 2020 20:33
@SammyK
Copy link
Contributor Author

SammyK commented Sep 25, 2020

Here's a possible solution to extensions that juggle errors like ext/soap: 687b7c1.

The idea is that for fatal errors, functions are "unobserved" after the end handlers fire for the first time. This causes all the observer handlers to fire and close prematurely for these specific edge cases, but ensures they only fire once.

The long term solution would be: fix each individual edge case of fatal error juggling.

@SammyK
Copy link
Contributor Author

SammyK commented Oct 6, 2020

@nikic I've looked at this a lot of different ways to see if we can get something akin to ZEND_HANDLE_EXCEPTION for remaining stack frames on shutdown after a zend_bailout. I don't think there is a safe way to traverse the stack on shutdown which makes me think we're stuck with the above solution of a one-time call to the end handlers on fatal errors.

I'm open to other ideas of addressing zend_bailout (ab)use. /cc @morrisonlevi @beberlei

@SammyK
Copy link
Contributor Author

SammyK commented Nov 10, 2020

Closing this in favor of #6377 which provides a more generic solution for handling zend_bailout.

@SammyK SammyK closed this Nov 10, 2020
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

Successfully merging this pull request may close these issues.

4 participants