Problem/Motivation
When creating a cache.$bin_name service, documentation for the cache API implies (but does not assert) that the service arguments: should be set to [$bin_name].
However, nothing in the code enforces this, and it's entirely possible to configure a cache service with service name and bin name different e.g:
services:
cache.foo:
class: Drupal\Core\Cache\CacheBackendInterface
tags:
- { name: cache.bin, default_backend: cache.backend.memory }
factory: cache_factory:get
arguments: [foo_bar]
This service will register fine, and appear to work fine, but will suffer from undefined behaviour; for example, its default_backend will not be respected.
The problem can be demonstrated with the following class:
namespace Drupal\cacheproblem;
class CacheProblem {
public function __construct(CacheBackendInterface $cacheBackend) {
$this->cacheBackend = $cacheBackend;
}
public function test() {
print "Previously:\tupdate=" . $this->cacheBackend->get('update') . "\n";
$string = date('c');
$this->cacheBackend->set('update', $string);
print "Cached update:\t$string\n";
print "Now set to:\tupdate=" . $this->cacheBackend->get('update') . "\n";
}
}
With the correct memory backend, this should not remember cached values between requests i.e. all calls should be the same as the first one.
With cache.foo and arguments: [foo]
This response reflects the correct behaviour:
$ drush php-eval '(new Drupal\cacheproblem\CacheProblem(\Drupal::cache("foo")))->test();'
Previously: update=
Cached update: 2017-01-20T15:30:18+00:00
Now set to: update=2017-01-20T15:30:18+00:00
$ drush php-eval '(new Drupal\cacheproblem\CacheProblem(\Drupal::cache("foo")))->test();'
Previously: update=
Cached update: 2017-01-20T15:30:23+00:00
Now set to: update=2017-01-20T15:30:23+00:00
$ drush php-eval '(new Drupal\cacheproblem\CacheProblem(\Drupal::cache("foo")))->test();'
Previously: update=
Cached update: 2017-01-20T15:30:28+00:00
Now set to: update=2017-01-20T15:30:28+00:00
The cache never "warms up", because it's correctly in memory.
With cache.foo and arguments: [foo_bar]
This response reflects incorrect/undefined behaviour, when the first argument does not match the service name:
$ drush php-eval '(new Drupal\cacheproblem\CacheProblem(\Drupal::cache("foo")))->test();'
Previously: update=
Cached update: 2017-01-20T15:29:34+00:00
Now set to: update=2017-01-20T15:29:34+00:00
$ drush php-eval '(new Drupal\cacheproblem\CacheProblem(\Drupal::cache("foo")))->test();'
Previously: update=2017-01-20T15:29:34+00:00
Cached update: 2017-01-20T15:29:38+00:00
Now set to: update=2017-01-20T15:29:38+00:00
$ drush php-eval '(new Drupal\cacheproblem\CacheProblem(\Drupal::cache("foo")))->test();'
Previously: update=2017-01-20T15:29:38+00:00
Cached update: 2017-01-20T15:29:54+00:00
Now set to: update=2017-01-20T15:29:54+00:00
The cache has (incorrectly) warmed up between requests, because it is now storing the data in a cache_foo_bar SQL table, not in memory:
mysql> SELECT * FROM cache_foo_bar;
+--------+---------------------------+--------+----------------+------------+------+----------+
| cid | data | expire | created | serialized | tags | checksum |
+--------+---------------------------+--------+----------------+------------+------+----------+
| update | 2017-01-20T15:44:41+00:00 | -1 | 1484927081.763 | 0 | | 0 |
+--------+---------------------------+--------+----------------+------------+------+----------+
1 row in set (0.00 sec)
Proposed resolution
How this can be resolved isn't clear, but behaviour should be consistent and undefined behaviour warned about:
* Either the DI container needs to spot that the configuration is inconsistent (if that's even possible) and abort
* Or the cache & backend integration needs to be impervious to the DI service name.
Remaining tasks
1. Work out what the tasks are!
User interface changes
None.
API changes
None (this is undefined behaviour.)
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | when_cache_x_service_is-2845358-8.patch | 5.39 KB | jp.stacey |
| #7 | when_cache_x_service_is-2845358-7.patch | 1.64 KB | jp.stacey |
Comments
Comment #2
jp.stacey commentedComment #3
jp.stacey commentedComment #5
jp.stacey commentedLooking at this as part of the Sprint Weekend 2017.
Comment #6
jp.stacey commentedHere's some of the things I've found out:
1) when caches are rebuilt,
ListCacheBinsPass::process()retrieves all services taggedname: cache.binand uses the service ID (e.g.cache.Xto determine the bin-to-backend correspondence. These compiled settings are then added as a parameter on the container.2) when a cache service is retrieved, the factory method
CacheFactory::get()is called witharguments: [Y], using the zeroth argument (e.g.Y) as the bin name. It then uses this bin name to try to look up the backend from the settings parameter compiled above.This is where the inconsistency arises. What we do about it is another question! Respectively:
1) when caches are rebuilt, only the service ID and the "attributes" (the tags) are currently available. But maybe more of the configuration could be discovered for each service ID in turn, and the arguments checked against the bin name?
2) when a cache service is retrieved, the original service ID is never passed to the factory method. So we can't detect an inconsistency here.
3) entirely separately, the status report page could warn of inconsistent cache configuration.
Comment #7
jp.stacey commentedPatch attached, making
ListCacheBinsPass::process()test for consistency.There's no existing test for this class that I can extend, but maybe it could be added as part of this issue if I've got time.
Comment #8
jp.stacey commentedPatch attached: the same changes in executed code; but with a new test.
The new test covers most of the existing
ListCacheBinsPass::process()behaviour, then also at the end tests what happens when an inconsistent service definition is passed in.I appreciate that my proposed resolution here in executed code—to strictly enforce consistency and raise an exception—might be up for discussion, but at least now it's easier to test the final behaviour by interdiffing from this patch.
Would appreciate any feedback!
Comment #20
catchSome validation seems sensible, but throwing an exception would potentially break existing sites as soon as they update. Maybe an assert() so it only breaks on local development?
Also think this mostly a DX improvement and not a bug as such, so downgrading to a task.