Skip to content

Make exit() unwind properly (minimal version) #5768

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 1 commit into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Jun 25, 2020

This is a minimal version of #5243. It switches exit() to throw an internal exception that is invisible to the user and only used to unwind the stack properly and perform a clean shutdown.

It does not execute finally block for exits, and leaves that issue for the future.

@ghost
Copy link

ghost commented Jun 25, 2020

Thank you, I'm all in for an internal exception with no userland behaviour change.

@morrisonlevi
Copy link
Contributor

When control returns to an internal function, will EG(exception) still be set? I'm really after this: will generic error handling code for internal functions work or break or..?

@nikic
Copy link
Member Author

nikic commented Jun 25, 2020

When control returns to an internal function, will EG(exception) still be set? I'm really after this: will generic error handling code for internal functions work or break or..?

Yes, it will be set. Internal functions will work correctly if they handle exceptions correctly (e.g., don't throw warnings if an exception has been thrown).

@nikic
Copy link
Member Author

nikic commented Jun 29, 2020

@dstogov Do you see issues with this change? This internally implements exit with an exception, to make sure we go through clean shutdown, and don't leak any persistent resources, when exit it used. It's intended to have minimal impact on user-visible behavior.

@dstogov
Copy link
Member

dstogov commented Jun 29, 2020

The idea looks fine, and I don't see any problems.

@php-pulls php-pulls closed this in 75a04ea Jun 29, 2020
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Aug 9, 2020
…estructAfterConstructorExit` sniff

> If an object constructor exit()s, the object destructor will no longer be
> called. This matches the behavior when the constructor throws.

Notes:
* This sniff will throw an error, even when there is no destructor as a destructor may be declared in a parent class.
    We could reduce the potential noise from this sniff a little further by only throwing the error when the class either contains a `__destruct()` method or extends a parent class.
* The name for the sniff feels a bit long. Suggestions for a better/shorter name welcome.

Opinions ?

Refs:
* https://github.com/php/php-src/blob/71bfa5344ab207072f4cd25745d7023096338385/UPGRADING#L200-L201
* php/php-src#5768 (I suspect, but not 100% sure)
* php/php-src@75a04ea
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Aug 10, 2020
…estructAfterConstructorExit` sniff

> If an object constructor exit()s, the object destructor will no longer be
> called. This matches the behavior when the constructor throws.

Notes:
* This sniff will throw an error, even when there is no destructor as a destructor may be declared in a parent class.
    We could reduce the potential noise from this sniff a little further by only throwing the error when the class either contains a `__destruct()` method or extends a parent class.
* The name for the sniff feels a bit long. Suggestions for a better/shorter name welcome.

Opinions ?

Refs:
* https://github.com/php/php-src/blob/71bfa5344ab207072f4cd25745d7023096338385/UPGRADING#L200-L201
* php/php-src#5768 (I suspect, but not 100% sure)
* php/php-src@75a04ea
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.

3 participants