In #3592577: Ensure that hook attributes are never parsed from a stale opcache we added an opcache_invalidate() call during hook parsing to ensure that hooks can never be parsed from a stale opcache.
The same client that ran into this problem and applied that patch recently ran into a similar issue.
A hook was moved from module A to module B, but hook invocation was trying to execute the hook from its original location in module A - where it doesn't exist so CallableResolver threw an error. This suggests that the key/value entry was somehow not updated correctly during the container rebuild.
There are broadly two ways that could happen:
1. Somehow the key/value item doesn't get updated at all, or is updated with stale data, this was the idea behind [##3592577: Ensure that hook attributes are never parsed from a stale opcache
2. The key/value item gets updated correctly by the container rebuild, but then gets updated again with stale data by another process.
I think it would theoretically be possible for #2 to happen in the following scenario:
Server 1 with deployment_identifier A starts a container rebuild (for whatever reason)
A code deployment completes on server 2, updating the code to a new version and deployment identifier B, triggering another container rebuild - this writes hook data to key/value before server 1.
Server 1 writes hook data to key/value at the end of its own container rebuild - and it's now stale.
Unlike the container cache key, the key value entries are always saved with the same key, so they are technically not guaranteed to correspond to a particular container.
Not sure, couple of ideas:
1. Move the hook info to a regular cache item, and build it on demand when it's empty, completely separate to container rebuilds. This would decouple hook discovery from container builds and generally mean it happens slightly later than it currently does. #3592624: [PP-1] Move hook parsing from container rebuild to just-in-time
2. Either with a raw cache entry or key/value, use the container cache key/deployment identifier for the key value keys as well, so that they always match a container. This would require garbage collection for older hook info on cron or similar though.
3. We could move the hooks back into the container - reversing the move to key/value. When the container is in the database this causes the container to be massive, but it might be less of an issue along with #3583505: Use Symfony PhpDumper instead of a serialized array container structure
| Comment | File | Size | Author |
|---|
Show commandsStart 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:
changes
Comments
Comment #2
catchComment #3
nicxvan commented2 seems like the best option to me I think
Comment #5
catchFor #2 We can't use the kernel container cache key because that varies more than the codebase does, but I think $settings['deployment_identifier'] / Drupal::VERSION is OK for this - as far as we know it's purely a deployment issue.
Pushed up a proof of concept that mostly passes tests - one unit tests I wasn't able to figure out yet.
Not sure this is where we want to end up in the long run, but it should help diagnose where it really is a container/hook mismatch on the client install that found this. And if it is, then we might want to commit it and make bigger changes in a follow-up since the other options here will be quite significant.
Comment #6
catchjs test failures look random.
Comment #7
nicxvan commentedThis looks great! I guess my only question, is do we need to worry about cron timing?
I'm a little surprised there isn't an update needed in ThemeHookCollectorPassTest
Comment #8
catchFor cron timing. It seems possible that if cron ran on a stale codebase just as new code was being deployed it could delete the k/v entry for the new hook entries because it was running with the old ones. To prevent that I think we'd need a lock but not sure how to do that within a container rebuild and in such a way it can be checked outside a container rebuild, which cron would need to do.
Comment #9
nicxvan commentedWouldn't we need to just lock the cron running when a container is being built?
I think we can probably open a follow up to discuss further since even if it is another potential race condition this is still better than the current situation.
The difference is the current race condition results in only a partial corruption, whereas the cron race condition would result in all hooks being missing, so maybe if there are no hooks we need to trigger a recollection somehow.
Comment #10
catchYes but how would we know a container is being built? A lock would be good but we'd need to use the lock backend at the start of the container rebuild.
Comment #11
catchPushed a commit to add a lock, in DrupalKernel::compileContainer() which I think answers the question of how we know a container is being built - e.g. just before it's built.
However this isn't 100% foolproof as far as I can tell - if a container rebuild finishes just before garbage collection happens in cron, then it could still garbage collect with the wrong identifier.
Then I thought about it a bit more, and was wondering about option 1 or 3 again, but might have something better than a lock:
When we write the hook data, we can write an extra key for the deployment_identifier - a single key to know which one was the last written.
Then on garbage collection, we can exclude both the identifier in key/value and the one in code, and that way if there are two 'current' identifiers, neither will get garbage collected.
Then on top of that, added the lock back, but with the key/value check on top, it ought to be enough hopefully.
Bumping to major because when this goes wrong, it can lead to fatal errors that can only be resolved via a manual cache clear.
Comment #12
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #13
catchComment #14
nicxvan commentedThis looks great, I think the HookData service should come first though so we only have to do the settings bit in one place.
#3618707: Add hook data cache service
Comment #15
ghost of drupal pastInstead of a lock and deployment identifier I thought we could add a version number to the container key. When a process wants to rebuild the container it first increases the version number using CAS (compare-and-set, see the SQL implementation below). If a process can't find the container with the given version number then they wait until one appears or until a (randomized) timeout when they try to do a rebuild themselves. This would go real well with #3583505: Use Symfony PhpDumper instead of a serialized array container structure.
So a process reads the current container version and when it wants to rebuild then it does
UPDATE SET container_version = $current_version +1 WHERE container_version = $current_versionsimilarly how queue does it and proceeds if the update succeeded. (Queue is getting a more modern single query but it doesn't apply here because the current version selection happens unconditionally earlier in the process.) Notably even Redis came around for CAS in 8.4.0 with SET IFEQ (Predis automatically supported it, phpredis has an issue but no support yet).The advantage vs a lock is this way of locking at the same ensures a stale container can no longer be read without extra database writes. Of course this means an extra database read for every container read.