fix: safe opcache_reset

[AlliBalliBaba]

Idea to fix #1737

Just a WIP, to test in CI.

[alexandre-daubois]

Hi @AlliBalliBaba, any news on this side? 🙂
I think I stumbled across the very same issue with #2265

[AlliBalliBaba]

I experimented a lot with juggling thread states, but not sure it's possible to make a reset fully safe from our side alone.

There are 2 main race conditions:

• A thread parses a script without having restarted after a opcache_reset has been triggered.
• Multiple threads trigger an opcache_reset simultaneously.

The first race condition kind of gets mitigated by the changes in this PR. THe second one would need to somehow be fixed in php-src or requires a hook to reject opcache_resets.

[withinboredom]

Can we try doing what we do with the environment functions and simply override/replace the existing one:

1. take a global write lock (read lock taken before executing a php script)
2. do the real reset
3. release the lock

[AlliBalliBaba]

Yeah I tried that as well. But an opcache_reset can also happen for other reasons, like after calling opcache_invalidate repeatedly because memory is filling up.

But maybe overwriting the user function would still be a good first step 👍

[henderkes]

Can we try doing what we do with the environment functions and simply override/replace the existing one:

1. take a global write lock (read lock taken before executing a php script)
2. do the real reset
3. release the lock

I tried this and it kind of works, but workers are an issue. Only got it working by locking new requests, draining all workers, doing the reset, restarting all workers and then unlocking.

[AlliBalliBaba]

Oh I just realized that overwriting opcache and restarting workers was also exactly what this branch does. Do you hard-link against the opcache extension in your solution @henderkes? When hard-linking and calling the reset directly, this can probably be done much more cleanly.
Simply calling the original reset function will make some Docker image builds still fail with segfaults.

[henderkes]

Yes, I hard linked because opcache is always active in 8.5+ (which is where I test). I'll tidy up my changes and merge them on top of this branch when I get to it.

[AlliBalliBaba]

Maybe we can make it a requirement also for other PHP versions, not sure how many installations there are without it.
Solution would still be the same, all threads need to stop completely, but we would force a flush instead of letting PHP schedule one.

[henderkes]

You already had logic to check whether opcache exists in the branch so I only had to pick in a minor change here. Let's see if tests pass.

Edit: well, apparently not. I only tested with 8.5 locally and guess what's passing x)

Okay, I think I get the issue now. Hard linking to opcache won't help because the init chain for shared extensions is different than for static extensions, which is why it works for me locally, but not in CI in < 8.5. Need to wait with actually calling opcache reset until the extension is properly loaded.

[henderkes]

Alright, we're down to php 8.2 segfaulting. I'm not terribly keep to find out why.

[henderkes]

Down to only debian 8.2 failing...

[AlliBalliBaba]

@henderkes do you remember what exactly fixed 8.2 for you before? I think I re-introduced the failure for some 8.2 versions while fixing a different race.

[henderkes]

@henderkes do you remember what exactly fixed 8.2 for you before? I think I re-introduced the failure for some 8.2 versions while fixing a different race.

It turned out that the failures were unrelated to the branch changes. I merged main so if it's still happening now it'll be something different.

[henderkes]

    caddy_test.go:1790: failed to call server Get "http://localhost:9080/sleep.php?sleep=11&work=11": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
    caddy_test.go:1779: failed to call server Get "http://localhost:9080/opcache_reset.php": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
    caddy_test.go:1790: failed to call server Get "http://localhost:9080/sleep.php?sleep=22&work=22": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
    caddy_test.go:1790: failed to call server Get "http://localhost:9080/sleep.php?sleep=26&work=26": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
    caddy_test.go:1790: failed to call server Get "http://localhost:9080/sleep.php?sleep=21&work=21": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
    caddy_test.go:1790: failed to call server Get "http://localhost:9080/sleep.php?sleep=20&work=20": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
    caddy_test.go:1790: failed to call server Get "http://localhost:9080/sleep.php?sleep=23&work=23": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
    caddy_test.go:1790: failed to call server Get "http://localhost:9080/sleep.php?sleep=27&work=27": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
    caddy_test.go:1790: failed to call server Get "http://localhost:9080/sleep.php?sleep=25&work=25": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
    caddy_test.go:1790: failed to call server Get "http://localhost:9080/sleep.php?sleep=24&work=24": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
    caddy_test.go:1779: failed to call server Get "http://localhost:9080/opcache_reset.php": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
    caddy_test.go:1779: failed to call server Get "http://localhost:9080/opcache_reset.php": context deadline exceeded (Client.Timeout exceeded while awaiting headers)

Not sure why... I've ran the exact same docker container tests 100x locally and didn't run into the failure a single time.

[Juoper]

Hey, quick question, would be really great if we could get this PR merged soonish, because it's blocking a lot of our team members. Is there any estimation for how long this takes to fix or can we assist in some way?

[Juoper]

so i even checked out this branch and built the docker image for it locally, but i still run into zend_mm_heap corrupted errors
is there a nice and easy way to help you guys to investigate the issue?

[henderkes]

zend_mm_heap_corrupted is generally an error in upstream php-src or caused by a not-fully-zts-compatible extension. This PR here would fix segfaults, rather than zend memory manager logic errors

[AlliBalliBaba]

@Juoper can you also try building from this branch: #2364

[gempir]

Is there anything I can do to help get this merged?

Properly reloading frankenkphp is of high priority to me (#2266)

[AlliBalliBaba]

@gempir are you able to build FrankenPHP from this branch #2364 (eg via Docker)? The PR should be ready to merge, but testing on more actual projects wouldn't hurt.

[gempir]

@AlliBalliBaba I compiled the current branch and ran it on our test system with our symfony application. I had no issues.
Anything specific I should test?

$ ./frankenphp-x86_64 --version
FrankenPHP vdev PHP 8.5.7 Caddy v2.11.4 h1:XKxkMTgNSizEvKG6QHue6cAsFOteU2qA61w2tKkCWi0=

[AlliBalliBaba]

Yes just specific problems you have been having regarding opcache. Or the watcher in general. People mainly have been reporting crashes during wordpress or moodle installation (like #1737).

[gempir]

Zero problems with opcache. But our symfony app is pretty clean and relatively new version, so probably a lot less troublesome to run than Moodle or Wordpress