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

Flaw in redis backend architecture #18

Open
vividvilla opened this issue Jul 24, 2018 · 2 comments
Open

Flaw in redis backend architecture #18

vividvilla opened this issue Jul 24, 2018 · 2 comments

Comments

@vividvilla
Copy link

vividvilla commented Jul 24, 2018

In recent redis backend update it seems like session fields are stored as a individual key value pair where value is a JSON encoded bydefault (default encode/decoder) and are namespaced as sessionid_fieldname.

Methods like Clear uses redis SCAN with count 9999999999 to find all the fields and clear it but this is a bad approach since redis SCAN is a blocking command and in a system with reasonable number of session keys blocking time is considerably high. I have tested this by inserting few million fake session keys in redis and called Clear method which took several seconds to return.

I think the better approach is to use redis hashmap for storing session. Session fields will be individual keys in the map and values are encoded with default encoder. Clear method can just delete the hashmap to clear the session in backend instead of iterating over keys. Performance of GetAll method all will be improved by redis HGETALL command which returns all the keys for given session id.

I can send a PR to address the above issue. Please let me know what you think.

@kataras
Copy link
Owner

kataras commented Aug 14, 2018

We used that approach in the previous release, I know the benefits and the downsides. You can create a PR and I can review it, maybe you can do it better with redis, I have years to use redis in my own products or on work.

@vividvilla
Copy link
Author

vividvilla commented Aug 16, 2018

Created a pull request (#19) with a fix. Pardon me but I didn't get you in above reply of yours

maybe you can do it better with redis, I have years to use redis in my own products or on work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants