Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cache] Symfony PSR-6 implementation #17408

Merged
merged 1 commit into from Jan 19, 2016

Conversation

Projects
None yet
@nicolas-grekas
Copy link
Member

commented Jan 17, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

From the provided README.md:

This component provides a strict PSR-6 implementation for adding cache to your applications. It is designed to have a low overhead so that caching is fastest. It ships with a few caching adapters for the most widespread and suited to caching backends. It also provides a doctrine/cache proxy adapter to cover more advanced caching needs.

Todo:

  • add more tests
  • add a FilesystemAdapter

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache branch 3 times, most recently from a046b58 to 42e7c97 Jan 17, 2016

{
$now = time();
if (null === $expiration) {

This comment has been minimized.

Copy link
@vlaurier

vlaurier Jan 17, 2016

I guess you wanted to write $time, and not $expiration

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache branch 2 times, most recently from c9d7fae to 82324ea Jan 17, 2016

@andrerom

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2016

Interesting, like the simplistic approach, and especially that there is only one backend lookup for getItem call. But missing use of multi get / set / (...) to reduce backend roundtrips further on getItems(array) and batch deferred updates. And calls to hasItem and getItem (and opposite) will result in two lookups here, but last point is maybe to be expected given the spec.

I know @Nyholm, @aequasi and others have been working on this topic, and hope to be able to contribute from eZ as well.

Could be an opportunity to join forces.


Side: On design side would perhaps opt for following approach:

  • 2 layers, the service (api) psr6 layer, and the adapter/driver layer with more low level interfaces, to avoid conflicts with future PSR's
  • Aim to support Tagging for reliable cache expiry on entity changes in most real world applications
@Nyholm

This comment has been minimized.

Copy link
Member

commented Jan 17, 2016

Thank you for the ping.

I ran this implementation on our integration tests and found some issues. Like you do not protect for all the reserved characters and you have forgotten to clear the deferred items on $pool->clear().

The php-cache supports tagging, hierarchal keys and have doctrine bridges in both directions. We are currently outperforming other PSR-6 caches with a factor of 2 and we have ideas for even more performance improvements.

We would definitely like to join forces somehow. We are open for a discussion about that.

@@ -0,0 +1,8 @@
Symfony PSR-6 implementaton for caching

This comment has been minimized.

Copy link
@pborreli

pborreli Jan 17, 2016

Contributor

implementaton => implementation

} elseif (is_int($time)) {
$time += $now;
} else {
throw new InvalidArgumentException(sprintf('Expiration date must implement DateTimeInterface or be null, "%s" given', is_object($time) ? get_class($time) : gettype($time)));

This comment has been minimized.

Copy link
@sstok

sstok Jan 17, 2016

Contributor

Exception message is wrong

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache branch 3 times, most recently from b9d7ef8 to c85b7dc Jan 17, 2016

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2016

Comments addressed, thank you @Nyholm for the link to the integration test, this will definitely help.
As I tried to state in the README, I target a strict implementation of PSR-6 here. The goal is not to replace any existing caching libraries (php-cache, stash, doctrine/cache, etc.) but only provide the features that have be standardized, so that we can build interoperability on top. This means that adding more interfaces or features (tagging) is out of scope, unless a new PSR standardize them.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache branch from c85b7dc to 46f688b Jan 17, 2016

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2016

I added testing with cache/integration-tests, I'll look at the failures asap. I already fixed the missing : reserved char.

@Nyholm

This comment has been minimized.

Copy link
Member

commented Jan 17, 2016

I target a strict implementation of PSR-6 here. The goal is not to replace any existing caching libraries (php-cache, stash, doctrine/cache, etc.) but only provide the features that have be standardized, so that we can build interoperability on top.

If the goal is not to replace any existing cache libraries, why build another cache library? Wouldn't it be better if all Symfony components using cache relied on the PSR6 interface and then required the virtual package psr/cache-implementation: ~1.0?

That way we use the benefit of PSRs and we can let the user decide what cache library to use. If they like to use feature X they install a library with feature X. If they want a strict implementation they would choose a strict implementation.

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 17, 2016

@Nyholm ... and this component is a strict implementation that users can use if they want to, like any other ones. The goal of PSRs (if I get this right) is to allow several implementations with different philosophies, not to standardize on only one.

@Nyholm

This comment has been minimized.

Copy link
Member

commented Jan 17, 2016

@Nyholm ... and this component is a strict implementation that users can use if they want to, like any other ones. The goal of PSRs (if I get this right) is to allow several implementations with different philosophies, not to standardize on only one.

You are correct. That is the goal of PSRs. But it was this sentence I reacted to:

The goal is not to replace any existing caching libraries

If the goal is to provide a strict implementation of PSR-6 that will replace (or be an option to) any existing library, then I agree that we need this component. Otherwise I do not see the benefits at all.

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 17, 2016

Understood, we definitely want to provide a strict implementation that can replace existing libraries :)

@Nyholm

This comment has been minimized.

Copy link
Member

commented Jan 17, 2016

Okey, awesome.

Im happy to help. @nicolas-grekas do you want PR's with more adapters now or once this is merged?

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2016

@Nyholm thank you that's much appreciated. Your test suite is already an amazing help. I think it would be easier to add more adapters after this one has been merge.
For now I have the following failures:

  1. Symfony\Component\Cache\Tests\CacheTest::testSaveWithNoData
    When saving an object with no data, we should generate an error.

I didn't read anything about this in PSR-6, did I miss something?

For the other ones, I'll check tomorrow hopefully:

  1. Symfony\Component\Cache\Tests\CacheTest::testDeferredSaveWithoutCommit
    A deferred item should automatically be committed on CachePool::__destruct().

  2. Symfony\Component\Cache\Tests\CacheTest::testExpiration
    Failed asserting that true is false.

  3. Symfony\Component\Cache\Tests\CacheTest::testIsHit
    Failed asserting that false is true.

  4. Symfony\Component\Cache\Tests\CacheTest::testIsHitDeferred
    Failed asserting that false is true.

@dunglas

This comment has been minimized.

Copy link
Member

commented Jan 17, 2016

Using a standardized interface for cache in Symfony will be very helpful!

What do you think about using the PSR-6 interface instead of the Doctrine Cache one in #16838 and #16917?

*/
public function save(CacheItemInterface $item)
{
if (!$item instanceof CacheItem) {

This comment has been minimized.

Copy link
@blanchonvincent

blanchonvincent Jan 18, 2016

Contributor

Why do you need to do this?
It breaks the usage of CacheItemInterface interface used as type hinting.

This comment has been minimized.

Copy link
@xavierleune

xavierleune Jan 18, 2016

Contributor

@blanchonvincent the PSR specifies that implementation are not compatible. So the type hinting is specified by the PSR interface, but the $item MUST be an instance from the same implementation (i.e: I cannot make work my CacheItem from my custom Redis implementation to work with your custom Memcached implementation of CacheItemPool).

This comment has been minimized.

Copy link
@aurimasniekis

aurimasniekis Jan 18, 2016

Contributor

You breaking the whole idea of PSR-6 you can interchange multiple libraries together... That's why Interfaces exists

This comment has been minimized.

Copy link
@dunglas

dunglas Jan 18, 2016

Member

@GCDS did you read @xavierleune's comment just in top of yours?

This comment has been minimized.

Copy link
@Nyholm

Nyholm Jan 18, 2016

Member

From the PSR itself:

Calling Libraries SHOULD NOT assume that an Item created by one Implementing Library is compatible with a Pool from another Implementing Library.

This comment has been minimized.

Copy link
@aurimasniekis

aurimasniekis Jan 18, 2016

Contributor

Ahh, sorry I thought differently...

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 18, 2016

Author Member

BTW, Interoperability between implementations is possible using something like the attached ProxyAdapter

@Nyholm

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

  1. When saving an object with no data, we should generate an error.

We have given some thoughts in to this. The PSR doc block on save says "True if the item was successfully persisted. False if there was an error.". I claim it is an error because null is valid data.

Consider this:

$item = $pool->getItem('key');
$pool->save($item); // Assume this is okey

$pool->hasItem('key'); // Should return true, yes? Or else, what is the point of saving?

/** 
 * Doc block for get(): 
 * If isHit() returns false, this method MUST return null. Note that null
 * is a legitimate cached value, so the isHit() method SHOULD be used to
 * differentiate between "null value was found" and "no value was found."
 * 
 * Doc block for isHit():
 * Confirms if the cache item lookup resulted in a cache hit.
 */
$pool->getItem('key')->isHit(); // Must be false because "no value was found."

// So ...
$pool->hasItem('key') !== $pool->getItem('key')->isHit(); // This makes no sense.

The pool should not have an item that never can be a hit. That would be a stale item and should be removed.
If you really want to store just the key you should just do $item->set(null);

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2016

@Nyholm I don't get your example, to me, assuming "key" is not already in the cache:

$pool->hasItem('key'); // false
$item = $pool->getItem('key');
$item->get(); // null
$item->isHit(); // false
$pool->save($item); // true
$item = $pool->getItem('key');
$item->get(); // null
$item->isHit(); // true -> a value has been found, we saved it before!
@Nyholm

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

You have not saved any value. Since: "Note that null is a legitimate cached value, so the isHit() method SHOULD be used to differentiate between "null value was found" and "no value was found.""

The last two rows in you example:

$item->get(); // null
$item->isHit(); // true -> a value has been found, we saved it before!

If $item->isHit() === true and $item->get() returns null you assume that someone has done a $item->set(null) which is not the case.

The point is, null should not be treated any special.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2016

If $item->isHit() === true and $item->get() returns null you assume that someone has done a $item->set(null) which is not the case.

My example above clearly allows to differentiate between a null a "found" vs a null as "not found". Nothing in the spec says that $item->set(null) is the only way to get a "data" null. This still looks like a non standard requirement to me.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache branch from 4e8b90c to 91e482a Jan 19, 2016

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 19, 2016

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 91e482a into symfony:master Jan 19, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jan 19, 2016

feature #17408 [Cache] Symfony PSR-6 implementation (nicolas-grekas)
This PR was merged into the 3.1-dev branch.

Discussion
----------

[Cache] Symfony PSR-6 implementation

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

From the provided README.md:

This component provides a strict [PSR-6](http://www.php-fig.org/psr/psr-6/) implementation for adding cache to your applications. It is designed to have a low overhead so that caching is fastest. It ships with a few caching adapters for the most widespread and suited to caching backends. It also provides a `doctrine/cache` proxy adapter to cover more advanced caching needs.

Todo:
- [ ] add more tests
- [ ] add a FilesystemAdapter

Commits
-------

91e482a [Cache] Symfony PSR-6 implementation

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:cache branch Jan 19, 2016

@Tobion

This comment has been minimized.

Copy link
Member

commented Jan 19, 2016

IMO namespaces handling should be a separate decorator and not built into the adapter directly. See doctrine/cache#96

@Tobion

This comment has been minimized.

Copy link
Member

commented Jan 19, 2016

I also want to express my objections against the recent fast merges of new components. This was merged in less than two days which does not give enough time to review and discuss a full new component at all. Also it's against our own rules which says

A pull request can be merged if: Enough time was given for peer reviews (at least 2 days for "regular" pull requests, and 4 days for pull requests with "a significant impact");
http://symfony.com/doc/current/contributing/code/core_team.html

I hope it's not going to be of the same quality as the LDAP component.

@andrerom

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2016

IMO namespaces handling should be a separate decorator and not built into the adapter directly. See doctrine/cache#96

True, it probably should be, but also a bit of a orange to apple comparison. In doctrine this is a namespace kept in cache and that needs to be checked on every cache lookup, that is not the case here (luckily, the extra cache lookup adds overhead, use case can be covered by tagging instead to avoid it).

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2016

@Tobion sorry if you feel uncomfortable, we merged quickly that's true... I invite you to open a PR to move the namespace concept in it's own decorator if you think it's better (taking @andrerom into account). The component will see more enhancements until 3.1 is released, but at least we don't have to discuss about its interfaces, which makes the situation totally different from the Ldap component hopefully.

if (!$item instanceof CacheItem) {
return false;
}
static $prefix = "\0Symfony\Component\Cache\CacheItem\0";

This comment has been minimized.

Copy link
@dunglas

dunglas Jan 19, 2016

Member

Why not a const?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 19, 2016

Author Member

because a const is public...

This comment has been minimized.

Copy link
@dunglas

dunglas Jan 19, 2016

Member

@internal? Or a private static property? This one looks a bit weird.

/**
* @author Nicolas Grekas <p@tchwork.com>
*/
class ProxyAdapter implements CacheItemPoolInterface

This comment has been minimized.

Copy link
@dunglas

dunglas Jan 19, 2016

Member

It deserves a little description of the usage IMO.

The ProxyAdapter class allows to use another cache item pool that complies with PSR-6 with the Symfony Cache component.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 19, 2016

Author Member

I added a small one in the README, could be better (and needs doc on symfony-doc :) )

}
static $prefix = "\0Symfony\Component\Cache\CacheItem\0";
$item = (array) $item;
$poolItem = $this->pool->getItem($item[$prefix.'key']);

This comment has been minimized.

Copy link
@dunglas

dunglas Jan 19, 2016

Member

It will throw an error because \0 isn't an allowed character in keys according to PSR-6.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 19, 2016

Author Member

you miss read: the $prefix is used locally to access the array cast, it's not given to the pool

This comment has been minimized.

Copy link
@dunglas

dunglas Jan 19, 2016

Member

Ok got it.

/**
* @author Nicolas Grekas <p@tchwork.com>
*/
class ArrayAdapter implements CacheItemPoolInterface

This comment has been minimized.

Copy link
@andrerom

andrerom Jan 19, 2016

Contributor

@nicolas-grekas Before considering to open PR on documenting this adapter better, what is your intention here?

  • A. Do you see it only for testing? Meaning:
    • We should clarify and avoid using it in doc examples.
  • B. Or supported in prod using a chain proxy in front of an actual cache pool? Meaning:
    • Needs to handle limits to avoid using all memory
    • Needs to reduce risk of returning stale data
    • Documenting that there will always be a risk of returning stale data (Unless we add flag to getItems to let users specify if they accept stale data or not, default not)

Context:
Stash has issues with this, the adapter is php documented as for unit tests only, but the bundle exposes an option to enable "InMemory" cache, w/o further details on the risk of broken application logic it might cause. See also: https://github.com/php-cache/array-adapter/issues/3

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 19, 2016

Author Member

@andrerom Your concern is filling memory with stale data or something more? There is no risk to return stale data, since the expiry is verified. The use case I see is when a non persistent in memory cache is enough. Could be testing or something else. We could enhance the logic for dealing with expirations, such has maintaining a property that stores the next full scan for expired items, to be checked e.g. on every access. Would you like to work on it?

This comment has been minimized.

Copy link
@andrerom

andrerom Jan 19, 2016

Contributor

Two issues:

  • There is no limit here so you run out of memory on long running scripts, could be a configurable argument with default set to 100 for instance
  • If you aim to support this for setups where there is any concurrence (several requests/processes/servers), and where invalidation cache is used when updates happen. Then the more concurrency you have + the longer your script runs, the higher likelihood of ending up getting stale data back from cache as local cache doesn't get invalidated.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 19, 2016

Author Member

I don't get what you mean by stale data: to me, data-staleness in the context of PSR-6 is defined by the expiry date. Until this date is reached, the data is not stale. After, it is. ArrayAdapter is not different from any other adapter when using this definition. The only concern I see that is specific to ArrayAdapter is garbage collecting the stale items. Do I miss something in what you said?

This comment has been minimized.

Copy link
@andrerom

andrerom Jan 19, 2016

Contributor

Expiry alone is worthless if you for instance use the cache service in a decorator for your persistence / data store ( Example ), in such a scenario you need to invalidate cache on changes to the data.

Feel free to suggest a better place to discuss this if you want.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 19, 2016

Author Member

a better place would be a new issue here on github :)

This comment has been minimized.

Copy link
@andrerom

andrerom Jan 19, 2016

Contributor

Here you go: #17445

@aurimasniekis

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2016

My suggestion for namespace would be to let Adaptor generate key with a namespace. For e.g. most Redis tools groups keys if they have : separators. If Adaptor for e.g. doesn't support some characters or uses own rules with namespaces it would be better I think in this way. Like I said for Redis e.g. to replace \ with : it would allow better viewing for redis tools...

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2016

@GCDS namespace (or prefix really) are already managed by adapters (they must deal with it in their constructor). Which means a Redis adapter could do whatever you'd need there. To me, the simple prefixing logic is rightfully in the abstract, because adding a prefix is really something common. Doing namespaces àla doctrine/cache is for a decorator, or at least not for the abstract, I agree.

@aurimasniekis

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2016

Ahh yeah, sounds correct to me :)

{
$this->namespace = $namespace;
$this->createCacheItem = \Closure::bind(
function ($key, $value, $isHit) use ($defaultLifetime) {

This comment has been minimized.

Copy link
@dunglas

dunglas Jan 19, 2016

Member

What about adding a constructor to CacheItem marked with @internal? It will be easier to understand. (Currently even PHPStorm isn't able to interpret this snippet and marks it in red).

This comment has been minimized.

Copy link
@dunglas

dunglas Jan 19, 2016

Member

Btw it will remove the duplication of this factory closure in ProxyAdapter.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 19, 2016

Author Member

I preferred raw speed over phpstorm compliance: a constructor would slow down things for no valid reason (adding one function call in the chain). I doubt @internal would be enough to prevent erroneous user side instantiation. The current no-constructor-nor-setter (for key and isHit) means we technically annoy users enough so that they are free to missuse but forced to think about it :)

This comment has been minimized.

Copy link
@dunglas

dunglas Jan 19, 2016

Member

Isn't a constructor call or a closure call in the chain similar in term of perfs?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 19, 2016

Author Member

yes of course, but making the constructor public has drawback IMHO (as detailed above). This drawback could be fixed by using a private constructor + a rebound closure, which is when an additional function call comes up. That's what I meant :)

@k00ni

This comment has been minimized.

Copy link

commented on .travis.php in 91e482a Jan 25, 2016

Isn't using @ with a function problematic, in case there is a critical error, which you suppress with using @?

This comment has been minimized.

Copy link
Member

replied Jan 26, 2016

There can only be errors when the URL isn't accessible, in which case the right part of the ?: operator will be executed.

@sebastianblum

This comment has been minimized.

Copy link

commented on 91e482a Jan 26, 2016

is there a plan to merge this feature into 2.8 and 3.0 or will it come with 3.1?

This comment has been minimized.

Copy link
Member

replied Jan 26, 2016

@sebastianblum both 2.8 and 3.0 are already released, so this new feature will make it into 3.1.

This comment has been minimized.

Copy link
Member

replied Jan 26, 2016

As it's a component and it doesn't rely on any other components, yo ucan use it without a problem in 2.8/3.0 apps btw.

@fabpot fabpot referenced this pull request May 13, 2016

Merged

Release v3.1.0-BETA1 #18776

@wouterj wouterj referenced this pull request May 27, 2016

Open

[WIP] [2.0] Introduced the concept of loaders #294

3 of 3 tasks complete
@lastzero

This comment has been minimized.

I'm trying to understand your code. Is this closure just to get access to the protected methods of CacheItem? Why not passing all values to the constructor to make them immutable and avoid setters, if that's the point? And since the cache item class is final, the class properties are in fact private, right?

/**
 * @author Nicolas Grekas <p@tchwork.com>
 */
final class CacheItem implements CacheItemInterface
{
    protected $key;
    protected $value;
    protected $isHit;
    protected $expiry;
    protected $defaultLifetime;
    protected $tags = array();
    protected $innerItem;
    protected $poolHash;

This comment has been minimized.

Copy link
Member

replied Feb 17, 2017

@lastzero we don't want to expose the initialization of these properties outside the cache pool. Instantiating a CacheItem yourselves is not a supported usage of PSR-6, and we want to enforce that.

Anyway, commenting on old commits is probably the worse way to get an answer: if we miss the email notification, we have no way to find your question in another way (we never go back in the github history to check all commits to see whether they have new comments).
for such question, asking on Slack is much better.

And using protected rather than private is to simplify some internal implementation details due to PHP features being used. It started as private.

This comment has been minimized.

Copy link

replied Feb 17, 2017

See, commenting on GitHub works :) I always read my GitHub mails. Thanks for sharing the insights!

PS: PHPStorm is getting confused when parsing your code. It marks it as error, this is how it got my attention.

@b-viguier b-viguier referenced this pull request Jul 28, 2017

Merged

add redis cache item pool adapter #39

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.