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

Redis caching/session enhancements #5519

Conversation

mv-steven
Copy link

@mv-steven mv-steven commented Dec 31, 2021

Add cluster support for Redis and Predis cache and session handlers. Add new session handler to use whatever the configured caching mechanism is.

Each pull request should address a single issue and have a meaningful title.

Description

  • Added cluster support for Redis and Predis cache and session handlers
  • Added new Predis session handler
  • Added new ConfiguredCacheHandler session handler to reuse whatever the configured caching mechanism is
  • Use php's serialization for getting/setting cache values rather than storing a separate hash for each object we store in the cache

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage -- changes have infrastructure dependencies, not sure how to address this
  • User guide updated
  • Conforms to style guide -- I couldn't find a style guide but I tried to follow the style that already existed

Testing
I tested against a redis server and redis cluster, using Redis/Predis as the cache/session handlers, then using the new ConfiguredCacheHandler session handler. Would like to setup unit tests but not sure how to handle that since there are infrastructure dependencies. Maybe in the future we can somehow include a docker command to spin up a redis image in unit tests.

if (! isset($data['__ci_type'], $data['__ci_value']) || $data['__ci_value'] === false) {
return null;
if(!(is_null($data = $this->redis->get($key)))) {
$data = unserialize($data);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storing php-serialized data is faster and more reliable than hard-coding the data types we anticipate.

@@ -204,19 +269,17 @@ public function close(): bool
{
if (isset($this->redis)) {
try {
$pingReply = $this->redis->ping();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remove the ping() call, because when a ping is unsuccessful/redis connection is broken, phpredis will throw an exception, just as it will with del() below.

@kenjis kenjis added the enhancement PRs that improve existing functionalities label Jan 1, 2022
@@ -28,13 +27,17 @@ class PredisHandler extends BaseHandler
* @var array
*/
protected $config = [
'scheme' => 'tcp',
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed 'scheme' since this is not exposed via Cache.php and is not used throughout this file. I have a future to-do for myself to add TLS support, then maybe we can reintroduce scheme here as well as in Cache.php and RedisHandler.php, and set it up to accept either tls or tcp.

default:
return null;
}
$data = $this->redis->get($key);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change at line 187 makes all the old type determination/storage unnecessary.

@mv-steven
Copy link
Author

mv-steven commented Jan 2, 2022

I noticed one of the workflows spat out this 'arcitecture violation' error:

CodeIgniter\Session\Handlers\ConfiguredCacheHandler must not depend on CodeIgniter\Cache\CacheInterface (Cache) /home/runner/work/CodeIgniter4/CodeIgniter4/system/Session/Handlers/ConfiguredCacheHandler.php:23

If this is a violation of the overall intended CI structure, feel free to remove ConfiguredCacheHandler, I can maybe set it up as a package that can be added on. We'll have to update the docs to remove this as well.

Also: There is indeed a decent amount of duplicated code for parsing cache configs, initiating phpredis/predis connections, etc. Happy to create common functions to handle this and eliminate dups. However, I'm concerned that this could cause another 'arcitecture violation' like ConfiguredCacheHandler does. Definitely open to suggestions/direction for where to put those common functions.

@mv-steven mv-steven marked this pull request as ready for review January 2, 2022 19:03
paulbalandan and others added 23 commits January 4, 2022 02:21
fix: Deserialization of Untrusted Data in old()
Co-authored-by: kenjis <[email protected]>
docs: add Security advisory in CHANGELOG.md
docs: minor addition to changelogs/v4.1.6.rst and upgrade_416.rst
Update modified Kint files in `ThirdParty`
…onnectDuration

fix: BaseConnection::getConnectDuration() number_format(): Passing null to parameter
kenjis and others added 24 commits January 21, 2022 09:23
Fix: Deletion timestamp of the Model is updated when a record that has been soft-deleted is deleted again
docs: Clarification when working with model pagination.
Co-authored-by: John Paul E. Balandan, CPA <[email protected]>
…hpunit

chore: generate coverage report by PHPUnit only on PHP 8.0
Signed-off-by: Andrey Pyzhikov <[email protected]>
fix: spark migrate:status does not show status with different namespaces
…ist-dot-notation

Refactor `if_exist` validation with dot notation
…empty-arrays

Fix `array_flatten_with_dots` ignores empty array values
Add cluster support for Redis and Predis cache and session handlers. Add new session handler to use whatever the configured caching mechanism is.
Changes after reviewing my PR
…ity improvements. Removed ConfiguredCacheHandler session handler, will add it separately. Predis cluster compatibility fixes.
@mv-steven
Copy link
Author

@kenjis pulled in latest develop, this is passing php-cs-fixer, rector, and phpstan on 7.4 and 8.1 in my local environment. There are some database tests failing locally but I did not touch anything database related.

@kenjis
Copy link
Member

kenjis commented Jan 24, 2022

@mv-steven It seems you did something wrong with git.
This branch has 274 commits now.
Please follow the instructions:
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#pushing-your-branch

@mv-steven
Copy link
Author

@kenjis I did follow those instructions. I'll setup a new, clean PR. But to be honest if this is going to continue to be such a painful process to get some relatively simple changes merged in, I'm gonna skip this and make this available as plugins.

@kenjis
Copy link
Member

kenjis commented Jan 24, 2022

@mv-steven I'm sorry, it seems you are not familiar with git.

I checked out your branch and followed the instruction.
I got confilcts:

$ git rebase upstream/develop
Auto-merging app/Config/Cache.php
CONFLICT (content): Merge conflict in app/Config/Cache.php
Auto-merging system/Cache/Handlers/PredisHandler.php
CONFLICT (content): Merge conflict in system/Cache/Handlers/PredisHandler.php
Auto-merging system/Cache/Handlers/RedisHandler.php
CONFLICT (content): Merge conflict in system/Cache/Handlers/RedisHandler.php
Auto-merging system/Session/Handlers/PredisHandler.php
CONFLICT (add/add): Merge conflict in system/Session/Handlers/PredisHandler.php
Auto-merging system/Session/Handlers/RedisHandler.php
CONFLICT (content): Merge conflict in system/Session/Handlers/RedisHandler.php
Auto-merging user_guide_src/source/libraries/sessions.rst
CONFLICT (content): Merge conflict in user_guide_src/source/libraries/sessions.rst
error: could not apply b13e43ba9... Redis caching upgrades
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply b13e43ba9... Redis caching upgrades

All you have to do is to resolve the conflicts, and compete the rebase command.

@mv-steven
Copy link
Author

@kenjis I did follow the instructions in the link you posted but I only saw 2 or 3 conflicts, which I resolved. But I do not claim to be a git expert :) Maybe there's some kind of local cache that was out of date. I'm going to squash my commits and open a new PR, hopefully that will clean everything up.

@mv-steven mv-steven closed this Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.