Skip to content

Conversation

twose
Copy link
Member

@twose twose commented Aug 14, 2022

This patch reverts part of d052742, d052742 makes Swoole/Swow can not work anymore, because Coroutine will yield to another one during socket operation, and EG(record_errors) assertion will always fail, and zend_begin_record_errors was only used during compile time before.
Use zend_try and zend_catch to fix it, and this patch also can be applied to PHP-8.0.

This patch reverts part of d052742, d052742 makes Swoole/Swow can not work anymore, because Coroutine will yield to another one during socket operation, and EG(record_errors) assertion will always fail, and zend_begin_record_errors was only used during compile time before.
Use zend_try and zend_catch to fix it, and this patch also can be applied to PHP-8.0.
@twose twose requested a review from bukka August 14, 2022 10:53
@twose
Copy link
Member Author

twose commented Aug 14, 2022

This is not a long-term solution (including zend_replace_error_handling, they are ugly but useful things), I think we need to stop trigger warnings everywhere without restrictions and use exceptions instead.

@bukka
Copy link
Member

bukka commented Aug 14, 2022

@twose This looks much better and nice test - didn't realise that I could actually use shutdown function for that. Can you separate it on revert and the actual fix so I can also apply it to 8.0?

@bukka
Copy link
Member

bukka commented Aug 14, 2022

just two commits in this PR basically - it will make it easier for me...

@bukka
Copy link
Member

bukka commented Aug 14, 2022

Or you can actually merge it yourself as you have access so if you could do that in that way, that would be awesome!

@twose
Copy link
Member Author

twose commented Aug 14, 2022

@bukka I can revert the previous patch on PHP-8.1, and apply this patch to PHP-8.0 manually, then merge it into all branches, is that OK?

@bukka
Copy link
Member

bukka commented Aug 14, 2022

@twose Yeah that would be perfect! Thanks for sorting that out.

@twose twose closed this in b8d0745 Aug 14, 2022
@twose twose deleted the fix-8409 branch August 14, 2022 12:22
@twose
Copy link
Member Author

twose commented Aug 14, 2022

@bukka I've done that, hope I did it correctly. Nice talking with you, thanks for your review!

@bukka
Copy link
Member

bukka commented Aug 14, 2022

Nice one. All looks good to me except just missing NEWS in 8.1 (we update NEWS in all active branches basically). Fixed in 7f64a8d

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.

2 participants