Problem/Motivation

In a persistent worker such as FrankenPHP in worker mode, the theme registry service can survive request cleanup. Registry::destruct() calls the runtime collectors' cleanup method but retains those objects. The inherited CacheCollector::destruct() persists pending changes and resets their storage.

After cleanup, Registry::getRuntime() returns the retained collector instead of constructing a fresh one. Its has() and get() methods read that emptied storage directly, so previously available theme hooks are no longer found. When rendering a single hook, ThemeManager::render() logs Theme hook %hook not found. and returns FALSE, leaving the themed output missing.

Steps to reproduce

  1. On a bootstrapped site, obtain the runtime collector using Registry::getRuntime() and confirm that it contains a registered theme hook.
  2. Call Registry::destruct() once.
  3. Obtain the runtime collector again for the same theme. It is the same object, and checking the previously registered hook now returns false.

Proposed resolution

In Registry::destruct(), copy the runtime collectors to a local variable and clear the service's collection before destructing the copied collectors. The next call to Registry::getRuntime() then creates a fresh collector, whose constructor loads the runtime cache or initializes the registry on a cache miss. Repeated cleanup also skips collectors that have already been processed.

Issue fork drupal-3626105

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

ptmkenny created an issue. See original summary.

ptmkenny’s picture

Status: Active » Needs review
amitgoyal’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the fix in MR !17253. Registry::destruct() now copies the runtime collectors to a local variable and clears $this->runtimeRegistry before destructing the copies, so a fresh collector is built on the next getRuntime() call instead of reusing the emptied one. Matches the proposed resolution.

Added test (testRuntimeRegistryAfterDestruct) confirms the theme hook is still found after destruct() and that a new collector instance is returned.

- Pipeline passes; the only red jobs are the PHPUnit Unit (Core/Component): [8.6-ubuntu] next-PHP-major lane, which is a non-blocking allow_failure job unrelated to this change.
- No open threads on the MR.
- Ran RegistryTest.php locally: OK (11 tests, 30 assertions).
- PHPCS clean on the diff; the pre-existing errors in Registry.php (lines 291, 461) also appear on unmodified main, unrelated to this change.

Moving to RTBC.

nicxvan’s picture

This is a duplicate of #3609212: Runtime ThemeRegistry left empty after destruct(), causing "Theme hook not found" warnings.

This one is a bit more concise and rtbc so even though it's newer I think we should postpone the other.

nicxvan’s picture

I think this looks good, this is at least the third duplicate I've seen.

Test only fails as expected, I think the reset first might be better.

Ultimately we want these to be memory caches and we likely need some updates for this to really work properly for persistent workers, but let's get this in so we can stop the duplicate reports.

RTBC +1

  • catch committed 3325c737 on 11.x
    task: #3626105 Release runtime theme registries after destruction
    
    By:...

  • catch committed 5dcbaebe on 12.0.x
    task: #3626105 Release runtime theme registries after destruction
    
    By:...

  • catch committed 96cfe413 on main
    task: #3626105 Release runtime theme registries after destruction
    
    By:...
catch’s picture

Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

So yes I think for persistent workers we might want to go the other way and not clean these up at all, so that any loaded theme registry is available to any other request without re-initialization from the persistent / chained fast cache. We could clean-up and rebuild only when cache items are updated etc. But we are not there yet, so let's make the current logic correct and then we can look at that later on.

Committed/pushed to main and cherry-picked to 12.0.x and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • longwave committed 8d9cd145 on 11.5.x authored by catch
    task: #3626105 Release runtime theme registries after destruction
    
    By:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.