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.
Comment #16
nicxvan commentedNeeds work for 15, seems reasonable to try that.
Comment #17
donquixote commentedSome thoughts about #15.
(I might add more later)
Nature of the problem
There could be a different problem:
- Request A starts with the old container.
- Request B starts a rebuild.
- Request B writes the container and the hook list.
- Request A reads the hook list from the new container, while it is still operating with assumptions from the old container.
Not sure how likely that is, it would mean that a lot of time passes between the request A starting and when it first gets the hook implementation list.
A more likely scenario might be this:
- Request B starts a rebuild.
- Request A starts with the old container.
- Request B writes the container and the hook list.
- Request A reads the hook list from the new container, while it is still operating with assumptions from the old container.
The second problem is being prevented by the incoming request wait proposal from #15, the first problem is not.
But the essence is that the mismatch can happen on read instead of write.
(more on that below)
Identifier uniqueness
The current MR uses the deployment identifier, which cannot distinguish different generations of the container from the same deployment.
It is not sufficiently unique in that sense.
The proposal in #15 solves this by introducing a counter that is unique to each container rebuild.
It also allows comparing which generation is newer, which we could not achieve with a random string.
I am not sure if we ever really need that, or if all we care about is uniqueness.
One problem I see is when we import a database dump that already has the counter incremented, could there be any confusion then?
An alternative would be a random string / uuid, or a unique hash.
The #3583505: Use Symfony PhpDumper instead of a serialized array container structure proposal already does introduce a unique name for the container based on a hash.
But we don't have that for the old container storage, we only have the deploy key which is not unique enough between rebuilds.
So could we introduce a similar value for the old-style container storage, and use that as a prefix for the hook list lookup?
Incoming request wait for new container?
In addition, #15 proposes to block incoming requests until a container rebuild is finished.
This seems reasonable, but is a bigger change that goes beyond the original problem.
Currently, (from what I know) an incoming request will use the old container if a new one is not available yet.
This is dirty, but has been the behavior for a long time.
Should we perhaps split that change to a follow-up?
So for now let incoming requests use the old container, then have a follow-up where we introduce the wait.
Or does the wait inherently become necessary by introducing the counter?
There is also a related scenario that is not being addressed here:
An incoming (regular) request starts with the old container.
Another request/process triggers a container rebuild.
The first request continues its operation with the old container, even if the rebuild is now in progress.
It might then get db query results with values that were written with the new container, such as the hook list.
Lifecycle mismatch
I see the main problem addressed in this issue the lifecycle mismatch of database key/value vs where the container definition lives.
Even if container definition and hook list are written at the same time, a request could read one version of the container but a different version of the hook list, if these reads happen at different times.
This problem can be addressed by having each external container asset being owned by one generation of the container, using a unique prefix as proposed.
Comment #18
nicxvan commentedI think the look ahead will solve this.
That requires a rebuild, it's no different than today, the hooks are stored in the keyvalue so you'd have outdated definitions anyway.
I read this to mean you agree with the proposed approach in 15?
Comment #19
donquixote commentedThe solution proposed in #15 will, in this case:
- "Request A starts with the old container" -> no change, because rebuild is not in progress yet.
- "Request B starts a rebuild" -> no change.
- "Request B writes the container and the hook list" -> no change.
- "Request A reads the hook list from the new container" -> this will change, because the keyvalue key would be prefixed with the new container index.
- Request A still will load _other_ data that is associated with the new container version.
We will achieve the same in that scenario with proposal #15 if we don't let incoming requests wait for the container rebuild.
There could still be funny edge cases. If we aim for a unique identifier per container rebuild, I would go for something that is unique across installations.
So...
Conclusion / proposed changes / questions
We should evaluate if a counter is the best we can do, or if instead we should use a random string or a hash.
A question is whether we ever need to know whether one is earlier or later, or if we only need to know if they are different.
Given that we already compute a hash for the php-generated container, we could do the same for the database container.
Aside of this, I propose to split the "let incoming requests wait for container rebuild" to a separate issue, for a more narrow scope of risks and consequences.
Comment #20
catchI was a bit concerned about a constantly incrementing container key because it would mean a lot more copies of the hook data in the database before they get cleaned up. However, it's a good point that #3583505: Use Symfony PhpDumper instead of a serialized array container structure will move to a unique container identifier anyway.
We don't need to know if a specific key is earlier than later, in the case of modules being uninstalled/installed simultaneously on production then it may be arguable which is 'first' anyway.
However, we do need to know which containers are a candidate for being the current container for garbage collection. If we can manage this for a random string, then that will be OK, but if not, then we might need to go for the CAS route. My preference would be the random string as long as garbage collection doesn't get too complicated.
I should hopefully have some time to work on this a bit this week.
Comment #21
ghost of drupal pastIt has been buggy the entire time. :)
Whether increment or random matters little the only way to do it is CAS anyways. You need to run UPDATE SET container_version = :something WHERE container_version = $current_version and whether something is $current_version + 1 or $randomstring matters not at all. As far as I can see, at least.
Comment #22
ghost of drupal pastPHP 8.1 introduced fsync. This makes a CAS class working on files possible. Together with the flat file dumper AFAIK this allows dropping the bootstrap container as the version is in a file.