search

AscendingCreations / AxumSession

Axum Session Management Libraries that use Sqlx
MIT License
136 stars 28 forks source link

SessionRedisPool Implementation Improvements #53

Closed wilgaboury closed 10 months ago

wilgaboury commented 10 months ago

I noticed a couple ways that the SessionRedisPool could be improved:

1) Using ConnectionManager (https://docs.rs/redis/0.23.0/redis/aio/struct.ConnectionManager.html) so that it is not establishing a new connection for every operation. 2) Utilize the table_name argument. It would essentially act as a prefix for session ids so that they don't conflict with other keys in the database.

I'm not sure how contributing is generally handled on this project, but I can make the changes and publish a PR if you'd like.

genusistimelord commented 10 months ago

feel free to publish a PR. Basically all contributions for new database types or improvements are very welcomed. I just review them all before pulling them in to ensure code quality, etc..

I think what really needs to happen is a connection Pool be created and handled for a Redis connection. Having a single active connection is good, but for a much larger site that has lots of concurrent access using a single connection could end up being bogged down. Using the ConnectionManager could make setting up a pool more possible. As Redis itself can accept many connections with no issue. Also Redis themselves might accept a PR for an internal Pool Type.

Here is a good example of a Connection Pool Manager. https://github.com/AscendingCreations/AxumOdbc/tree/main/src this should work very close to how Redis works in general. Feel free to start in the direction of using the ConnnectionManager first. Though this may be faster than creating a new connection every time this may also be slower when there is lots of concurrent access to the website. That is why I generally choose to create a connection each time to support heavy accessed websites.

I also do agree that Utilizing the table_name would be helpful here I just couldn't remember how Redis handles that exactly. Also I think adding a Contribution guide letting people know I welcome and review PR would be a good thing too.

wilgaboury commented 10 months ago

I see your point and agree that there are some drawbacks to using ConnectionManager, where as a connection pool seems like a better design choice. That being said without a built in async connection pool in redis-rs maybe it would be best to keep things as they are for now.

On the matter of table name, what I was originally envisioning is appending the table name to the beginning of each id, and changing some of the batch commands to iterate over keys using patterns (table_name*), but looking into it more, it doesn't seem like there is a efficient way to do patterned delete or count (currently using FLUSHDB and DBSIZE). For my personal use case it might be better to just have two separate redis instances, one for application data and one for sessions, instead of trying to mix the two into a single instance.

So I guess this issue can be closed :) Thanks for the feedback!

genusistimelord commented 10 months ago

I can work on making a Redis Pool later as an improvement for Redis. it doesn't have to be inside the Redis library. Also currently we are using the UUID as the current id to set the session too. It would be very hard for this to collide with other things in the Redis instance unless you to are using a UUID to store things. In this case I could append the Table to the front or back of the UUID which would make it more unique. but yes it doesn't hurt to have multiple Redis instances in this case. Also feel free to stop by my discord to discuss things there as well.

wilgaboury commented 10 months ago

Thanks, I just joined your discord. You're right that there's no reason to worry about key conflicts since the library is using random uuids, but I am worried that DBSIZE will give inaccurate counts and FLUSHDB will clear out important application data, though I haven't studied the code enough to understand what cases that gets called.

genusistimelord commented 10 months ago

FlushDB would only ever get called if you called clear_store which can only be called from the session_store. Yes the DBSIZE would be a bit inaccurate unless we can find a way to only get a specific portion of it via the table_name* stuff if Redis supports this. not entirely sure there.

genusistimelord commented 10 months ago

I will leave this open as to remind me to work on a Redis Pool.

genusistimelord commented 10 months ago

hey @wilgaboury would you mind testing out my redis pool library and changes within the session branch. https://github.com/AscendingCreations/AxumSession/tree/redispool

let me know if this is what you really needed =D.

wilgaboury commented 10 months ago

Thank you, this looks great! Maybe it would be worth making some automated integration tests for the redispool library using something like https://github.com/testcontainers/testcontainers-rs/ to spin up redis instances. I can make a PR for that if you like the idea.

genusistimelord commented 10 months ago

that would be a very welcomed PR. it would also help to test it on a multi request access stand point too. I even added the Cluster stuff to its own pool as well but I did not add it to sessions yet. I will once this is confirmed working fully.

genusistimelord commented 10 months ago

I am closing this issue since main now has the new redis_pool.