[Observer] Fire open observer end handlers after a zend_bailout #6377
Conversation
Zend/zend_observer.c
Outdated
| typedef struct _zend_observer_frame { | ||
| zend_execute_data *execute_data; | ||
| struct _zend_observer_frame *prev; | ||
| } zend_observer_frame; |
There was a problem hiding this comment.
I don't understand the purpose of this. Why can't we walk the current_execute_data at the time of bailout backwards?
There was a problem hiding this comment.
If we walked the stack right before the current_execute_data gets set to NULL, then all the end handlers will fire prematurely if the zend_bailout is "caught".
Instead of tracking each observed frame, we could just keep current_observed_frame as a pointer to the last observed execute_data, and walk the stack backward from that point during request shutdown. WDYT?
There was a problem hiding this comment.
Instead of tracking each observed frame, we could just keep current_observed_frame as a pointer to the last observed execute_data, and walk the stack backward from that point during request shutdown. WDYT?
That sounds reasonable to me.
There was a problem hiding this comment.
@nikic I gave this approach a shot; when a function call/include is finished, the end handler sets the current_observed_frame to the prev_execute_data. This works fine for all cases except for file includes.
Once the current_observed_frame is set to a file include on the last observed end handler, the op_array for that include is destroyed before php_request_shutdown is called (during a non-zend_bailout "happy path"). Then zend_observer_fcall_end_all tries to obtain the map pointer from the current_observed_frame's op_array that has already been freed.
Can you think of a different approach to address the issue of freed op_arrays, or should we go ahead with the existing solution?
There was a problem hiding this comment.
Does this affect only the top-level file or intermediate includes?
There was a problem hiding this comment.
It appears to be just the top-level file actually. I tried digging into it more deeply to figure out exactly how intermediate includes don't seem to have this issue. I didn't figure it out exactly, but it looks like perhaps the runtime cache thats pointed to from the execute_data is preserved even after the op_array is freed?
There was a problem hiding this comment.
I finally figured it out. We just needed to keep track of the first observed frame. Fixed in 8877bef. Let me know if you think this looks good for merge now. :)
SammyK
force-pushed
the
observer/zend_bailout
branch
from
1bd5377 to
ccde1e2
Compare
|
@nikic Are you 👍 if I merge this today to make sure it's fixed in RC4? |
|
@SammyK RC4 was already tagged: https://github.com/php/php-src/releases/tag/php-8.0.0RC4 |
|
Doh! Missed it by 7 hours. Thanks for the heads up. Are y'all planning to squeeze an RC5 back into the schedule? If not, that's fine - we can use custom builds in our CI for now. |
|
We have GA planned as the next one already in for Nov 26th (https://wiki.php.net/todo/php80), but I see no problems to release something next week again :) |
SammyK
force-pushed
the
observer/zend_bailout
branch
from
ccde1e2 to
8877bef
Compare
|
@carusogabriel If the latest changes in this PR are 👍 from @nikic, I would love to take you up on your offer to squeeze in an RC5 next week. Between this PR and #6421, there are quite a lot of bug fixes to the observer API that would be really great to land in an RC before GA. But no pressure if it's not possible. :) |
|
@remicollet @cmb69 Can you help me release an RC5 next week? I can create the tarballs, but I remember that I need you to run the QA stuff |
|
I could roll the Windows builds next week, but I think we need to adjust the release cycle anyway, since 7.4.14 and 8.0.1 are currently scheduled for December, 24th – does that ring a bell? ;) |
|
This fix seems to be worthy of putting RC5 back in the stack. One week on isn't too soon, so deffo go for that. As to how this impacts the GA: We'll see. I'm not against delaying GA if the fix has any question marks attached to it, but if y'all think it's stable and unconcerning, we can keep the current date of Nov 26th. |
Traditionally we've simply skipped a two-week cycle over the holidays. So we could roll 7.4.14/8.0.1 RC1 as planned on Dec 10th, then wait four weeks to release finals on Jan 7th which would be shortly after folks get back to work from the holidays. Letting an RC back a little longer hurts nobody. |
|
Btw, this PR is targeting |
Thanks! :)
I'll merge into @nikic Are you happy with the latest change for this to merge this week? |
|
@carusogabriel Do you have an idea of what day you're planning on tagging RC5? I'll make sure to get any observer PR's merged in before then. :) |
|
@SammyK tagging always happens on Tuesdays. |
|
I can do the tag until 18h CET, @SammyK what is your timezone? |
|
@carusogabriel Thanks for the heads up. I'm in Pacific Time, so I believe that's 9am for me. @nikic I should probably merge this and #6422 later this afternoon Pacific Time just to play it safe. |
This PR supersedes #6114. It ensures that the observer end handlers are fired after an arbitrary
zend_bailout(not just fatal errors).