Skip to content

Fix Segfault when using ReflectionFiber #10439 #10478

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
wants to merge 1 commit into from
Closed

Fix Segfault when using ReflectionFiber #10439 #10478

wants to merge 1 commit into from

Conversation

danog
Copy link
Contributor

@danog danog commented Jan 31, 2023

No description provided.

@devnexen devnexen requested a review from iluuu1994 January 31, 2023 17:38
@Girgias Girgias linked an issue Jan 31, 2023 that may be closed by this pull request
Segfault when using ReflectionFiber #10439
Closed
@Girgias Girgias changed the title Fix #10439 Fix Segfault when using ReflectionFiber #10439 Jan 31, 2023
@Girgias Girgias requested a review from trowski January 31, 2023 18:02
@trowski
Copy link
Member

trowski commented Feb 3, 2023

This LGTM. Apologies that I didn't catch this when writing the reflection API for fibers. I copied the Generator API for these methods, but yield cannot be used outside of a user function so doesn't have this weird edge case.

Can this be merged into 8.1? It's a BC break for the API, but the prior behavior here was to crash, so seems a reasonable trade-off? /cc @ramsey

@ramsey
Copy link
Member

ramsey commented Feb 3, 2023

I'm okay with this change for 8.1. @patrickallaert and @krakjoe, disagree or agree?

@danog
Copy link
Contributor Author

danog commented Feb 15, 2023

Ping? :)

iluuu1994
iluuu1994 approved these changes Feb 15, 2023
View reviewed changes
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

The implementation looks good to me.

@danog danog changed the base branch from master to PHP-8.1 February 22, 2023 18:45
@danog
Copy link
Contributor Author

danog commented Feb 23, 2023

Squashed and rebased this on PHP-8.1, should be good to merge.

iluuu1994
iluuu1994 approved these changes Feb 23, 2023
View reviewed changes
Copy link
Member

@iluuu1994 iluuu1994 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Implementation looks good to me! Would be nice if there was a test for the parent behavior. I think you could test this with array_map and ["Fiber", "suspend"]. Nevermind, this is obviously already tested.

@iluuu1994 iluuu1994 closed this in 8d1c0a1 Feb 23, 2023
@kocsismate kocsismate mentioned this pull request Apr 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwoebi bwoebi bwoebi left review comments

@trowski trowski trowski approved these changes

@iluuu1994 iluuu1994 iluuu1994 approved these changes

Assignees
No one assigned
Labels
Category: Fibers Extension: reflection
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Segfault when using ReflectionFiber
6 participants
@danog @trowski @ramsey @iluuu1994 @bwoebi @Girgias