[Cache] Implement PSR-16 SimpleCache #20636

nicolas-grekas · 2016-11-25T16:27:36Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	(not yet, and many are missing anyway)
Fixed tickets	-
License	MIT
Doc PR	-

Let's review PSR-16 by looking at a possible actual implementation of it, helping @Seldaek in his quest to pass it to the FIG.
See also:

Seldaek · 2016-11-25T16:34:10Z

I'm kinda confused here, I don't know the cache component in depth but it looks like you implemented CounterInterface in the adapters, but then to actually use the new CacheInterface you have to use the simple cache adapter on top of the PSR-6 cache pool, therefore losing the natively implemented CounterInterface?! Maybe you're just not finished yet :)

Seldaek · 2016-11-25T16:34:59Z

Ah no I missed this..

 +        if ($this->pool instanceof CounterInterface) {
 +            return $this->pool->increment($key, $step);
 +        }

All is well :)

nicolas-grekas · 2016-11-25T20:25:35Z

Questions sent to the FIG:

On CounterInterface:

the draft doesn't say what happens when $key is invalid => the same exception as PSR-6?
nor does it say what happens when $step is not an integer => return false? throw something?
what should happen when $key already exists in the storage but is not "incrementable"? (Redis INCR fails, I didn't check apcu) => return false? throw? erase and store $step? other?

On CacheInterface:

doesn't say what happens when $key is invalid => the same exception as PSR-6?
the fact that getMultiple returns all keys, even cache misses, makes it impossible to (efficiently) implement a PSR-16 to PSR-6 bridge, because it makes it impossible to detect cache misses. When given an array, apcu_fetch for example has this other behavior of returning only the "hit", so it doesn't suffer from this. Could this be worth considering?

dragoonis · 2016-11-25T20:34:25Z

Thanks you @nicolas-grekas for creating this PR. Saved me some time :-)

stof · 2016-11-25T20:43:21Z

src/Symfony/Component/Cache/Adapter/AbstractAdapter.php

+            }
+        }
+
+        $e = $this->doSave(array($id => $step), 0);


this implementation in the base adapter is suspicious. The spec asks for atomicity, but atomicity requires to overwrite this method in each child adapter. An adapter not overwriting it would not provide atomicity, thus breaking the spec.

see #20636 (comment)

stof · 2016-11-25T20:48:25Z

src/Symfony/Component/Cache/SimpleCache.php

+    public function getMultiple($keys)
+    {
+        foreach ($this->pool->getItems($keys) as $key => $item) {
+            yield $key => $item->get();


The spec says @return array, which excludes generators

on purpose @Seldaek?

Not really on purpose. Please email on fig ML to keep everything centralized there because otherwise stuff will get lost.. Not sure if I like generators here though, if you get multiple keys you most likely want to have them in an array where you access every result by key, and not have to loop over it.. But it's also kinda easy to wrap iterator_to_array so not huge deal either way.

stof · 2016-11-25T20:48:55Z

src/Symfony/Component/Cache/SimpleCache.php

+     */
+    public function getMultiple($keys)
+    {
+        foreach ($this->pool->getItems($keys) as $key => $item) {


this call would be done only when starting to iterate the generator, which is too late

stof · 2016-11-25T20:49:31Z

src/Symfony/Component/Cache/SimpleCache.php

+        if (is_numeric($value = $item->get())) {
+            $step += $value;
+        }
+        $item->set($step);


this fallback logic does not respect the spec, as it is not atomic

@Seldaek shouldn't the spec use a normative MUST or SHOULD on this topic? (SHOULD would be my pref so that fallback implems are kind of OK, it'd then be up to the user to chose an implem+backend that provides real atomicity when required).

there is only the phpdoc here in the spec, and it says Increment atomically so it looks like a MUST for now (at least I understand it this way when reading it)

yeah people complained about it being part of the CacheInterface initially because it wasn't possible to guarantee it can be implemented atomically.. So we split it, to keep it MUST, because otherwise it's kinda useless really IMO. I think it's still fine for implementors to provide non-atomic if they want, but at least violating the spec means you should warn people and make them aware that non-atomic ones are a bad idea if you do anything related to security or just overall don't want to lose data in high-concurrency settings.

nicolas-grekas · 2016-11-25T22:38:25Z

New unresolved issues with PSR-16 draft:

accepting Traversable for *Multiple methods is not consistent with PSR-6 which only takes arrays as arguments
returning array for getMultiple is not consistent with PSR-6 getItems which returns array|Traversable
atomicity in CounterInterface misses a normative MUST or SHOULD

staabm · 2016-11-26T15:04:42Z

src/Symfony/Component/Cache/Adapter/AbstractAdapter.php

+            if (is_numeric($result = $this->doIncrement($id, (int) $step))) {
+                return $result;
+            }
+            CacheItem::log($this->logger, 'Failed to increment key "{key}"', array('key' => $key));


Since you call this code also from decrement, this message may be misleading in some cases

nicolas-grekas · 2016-11-29T21:54:25Z

This PR achieved its goal to spot edge cases in PSR-16.
Closing to keep this version in the github history.
New PR to come when the next iteration of PSR-16 is ready.
Thank you all for the feedback.

…kas) This PR was squashed before being merged into the 3.3-dev branch (closes #20694). Discussion ---------- [Cache] Implement PSR-16 SimpleCache v1.0 | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | not yet | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#7409 Second iteration on the topic after #20636 raised some issues. Please don't squash while merging. Commits ------- 6219dd6 [Cache] Create PSR-16 variants of all PSR-6 adapters 99ae9d6 [Cache] Move adapter implementations to traits 848a33e [Cache] Implement PSR-16 SimpleCache v1.0

carsonbot added Status: Needs Review Cache Feature labels Nov 25, 2016

nicolas-grekas force-pushed the simple-cache branch from 40ea9fd to 47f44a8 Compare November 25, 2016 20:17

nicolas-grekas mentioned this pull request Nov 25, 2016

sefgault on 7.1 krakjoe/apcu#217

Closed

stof reviewed Nov 25, 2016

View reviewed changes

[Cache] Implement PSR-16 SimpleCache

858d93b

nicolas-grekas force-pushed the simple-cache branch from 47f44a8 to 858d93b Compare November 26, 2016 08:33

nicolas-grekas mentioned this pull request Nov 26, 2016

Proposal coming from Symfony trial implementation php-fig/fig-standards#846

Merged

staabm reviewed Nov 26, 2016

View reviewed changes

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Nov 26, 2016

nicolas-grekas closed this Nov 29, 2016

nicolas-grekas deleted the simple-cache branch November 29, 2016 21:54

nicolas-grekas mentioned this pull request Nov 30, 2016

[Cache] Implement PSR-16 SimpleCache v1.0 #20694

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cache] Implement PSR-16 SimpleCache #20636

[Cache] Implement PSR-16 SimpleCache #20636

nicolas-grekas commented Nov 25, 2016 •

edited

Loading

Seldaek commented Nov 25, 2016

Seldaek commented Nov 25, 2016 •

edited

Loading

nicolas-grekas commented Nov 25, 2016 •

edited

Loading

dragoonis commented Nov 25, 2016

stof Nov 25, 2016

nicolas-grekas Nov 25, 2016

stof Nov 25, 2016

nicolas-grekas Nov 25, 2016

Seldaek Nov 25, 2016

stof Nov 25, 2016

stof Nov 25, 2016

nicolas-grekas Nov 25, 2016 •

edited

Loading

stof Nov 25, 2016

Seldaek Nov 25, 2016

nicolas-grekas commented Nov 25, 2016 •

edited

Loading

staabm Nov 26, 2016

nicolas-grekas commented Nov 29, 2016

[Cache] Implement PSR-16 SimpleCache #20636

[Cache] Implement PSR-16 SimpleCache #20636

Conversation

nicolas-grekas commented Nov 25, 2016 • edited Loading

Seldaek commented Nov 25, 2016

Seldaek commented Nov 25, 2016 • edited Loading

nicolas-grekas commented Nov 25, 2016 • edited Loading

dragoonis commented Nov 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Nov 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Nov 25, 2016 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Nov 29, 2016

nicolas-grekas commented Nov 25, 2016 •

edited

Loading

Seldaek commented Nov 25, 2016 •

edited

Loading

nicolas-grekas commented Nov 25, 2016 •

edited

Loading

nicolas-grekas Nov 25, 2016 •

edited

Loading

nicolas-grekas commented Nov 25, 2016 •

edited

Loading