Skip to content

Conversation

@AlliBalliBaba
Copy link
Contributor

This is an alternative to #1799 that instead doesn't flush the $_ENV global at all. I realized that the 'default' state of globals is just a bunch of 0 bits. I also realized that worker mode currently doesn't support $_REQUEST.

cc @withinboredom can you check if this also fixes the segfaults you have been seeing?

@AlliBalliBaba
Copy link
Contributor Author

AlliBalliBaba commented Aug 21, 2025

I just now also realized that it's not even necessary to flush globals. Their re-import will flush them anyways.

Only $_FILES needs to be flushed or it will segfault, still curious why.

Also surprised that noone has so far complained about $_REQUEST not being supported in worker mode. It will only work if it's jit is disabled.

@AlliBalliBaba
Copy link
Contributor Author

AlliBalliBaba commented Aug 21, 2025

Alright, I think I finally figured out a cleaner way to reset globals.

Insterestingly, only $FILES will not be flushed on import, all other globals are flushed.
see: $_GET $_POST $_COOKIE $_SERVER $_ENV $_FILES $_REQUEST

I think it might make sense to document that $_REQUEST is only supported in worker mode if auto_globals_jit = Off (unrelated to this PR).

@AlliBalliBaba AlliBalliBaba marked this pull request as ready for review August 21, 2025 19:33
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

LGTM. Is it ready to be merged?

@AlliBalliBaba
Copy link
Contributor Author

Just merged main, should be good to go if CI is green

@AlliBalliBaba
Copy link
Contributor Author

AlliBalliBaba commented Aug 25, 2025

Watcher tests seem to be failing right now (maybe an update?), I'll have a look

@AlliBalliBaba
Copy link
Contributor Author

AlliBalliBaba commented Aug 25, 2025

Nvm, rather it looks like the phpheaders_test (#1823) will still randomly fail builds so I moved it back to frankenphp_test (probably will report this one to go)

@AlliBalliBaba
Copy link
Contributor Author

Should be fine now 👍 , linting failure is unrelated to this PR.

@AlliBalliBaba AlliBalliBaba mentioned this pull request Aug 26, 2025
@dunglas dunglas merged commit 952754d into main Aug 27, 2025
60 of 63 checks passed
@dunglas dunglas deleted the fix/dont-flush-env-between-requests branch August 27, 2025 06:30
@dunglas
Copy link
Member

dunglas commented Aug 27, 2025

Thank you! Great work, as usual.

henderkes pushed a commit to static-php/frankenphp that referenced this pull request Sep 11, 2025
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