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

Pass State into the Session Backend #467

Open
StephenWakely opened this issue Sep 4, 2020 · 9 comments
Open

Pass State into the Session Backend #467

StephenWakely opened this issue Sep 4, 2020 · 9 comments

Comments

@StephenWakely
Copy link
Contributor

StephenWakely commented Sep 4, 2020

I am working on a Redis backend for storing the session.

The way to do this, I imagine, is to create a separate Middleware to manage a connection pool to Redis. This Middleware would store a connection in the State that the Session backend can use. I don't see any other way that the Redis connections could be handled, correct me if I'm wrong.

In order for the backend to use the connection it needs to have access to the State. Currently, this isn't passed in. It would be great if it could be!

I have knocked up a quick proof of concept here: https://github.com/FungusHumungus/gothamboogy/blob/master/src/redis.rs, and have a tentative pull request available - if you think this is the correct way to solve this issue. I'll submit it in a minute for your perusal.

This is probably also relevant to #69.

@msrd0
Copy link
Member

msrd0 commented Sep 4, 2020

I just took a look at your proof of concept and I don't think I fully understand why you want your redis connection in the State. You don't seem to store a connection pool of any kind in either the middleware or the session backend, but create a new connection for every request. Even worse, you return a 500 if redis is not available before the handler even requests a session (which might be fine for your use case if every handler is guaranteed to require a session).

If I understand your description correctly, what you want is a connection pool for your session backend. Is there any reason why it is not possible for you to just store that in the backend but have to do so in a middleware? I'm not against passing the state to the session backend, just trying to understand why it's necessary.

@StephenWakely
Copy link
Contributor Author

So I just haven't gone to the trouble of setting up a pool in the POC, it's just easier to create a new connection every time - but ultimately that should be handled by a proper pool.

I agree, it's not ideal that to have a Redis backed session store, you have to then also create the Redis middleware, and horrible things would happen at runtime if you forgot to add the Redis middleware. But since (in my case anyway) the handlers will likely also want a Redis connection, it makes sense to use the same connection for the session and the handlers.

The alternative, as you suggested to create the pool and store it in the backend would certainly be possible. I suspect though, to get a connection from the pool would probably involve a future - as it does in darkredis. In which case the backend methods (persist_session and drop_session) would presumably also need to return a future.

I would be happy to have a go at getting that to work if you think it would be a better solution, or if you can think of any other better ideas?

@msrd0
Copy link
Member

msrd0 commented Sep 4, 2020

Ok I see so you're trying to have one redis connection pool that can be shared with the session backend and the handlers. In that case it probably makes sense to use a middleware, eventhough you'll need to ensure that the redis middleware will always run before the session middleware.

@msrd0 msrd0 linked a pull request Sep 4, 2020 that will close this issue
@StephenWakely
Copy link
Contributor Author

The more I think about it, the more I think it would be useful for all the backend methods (persist_session and drop_session) to return a future anyway. It just so happens that the Redis library I have been using doesn't return a future if there are no results to return - however a lot of other libraries would.

@msrd0
Copy link
Member

msrd0 commented Sep 5, 2020

Yes that would probably make sense and also be more consistent.

@StephenWakely
Copy link
Contributor Author

Would you like me to work that into the current pull request, or raise a separate issue for it?

@msrd0
Copy link
Member

msrd0 commented Sep 5, 2020

I think it fits into this issue/PR, so if you can include it into the current PR, that'd be cool.

@StephenWakely
Copy link
Contributor Author

Awesome. I'll see what I can do.

@msrd0 msrd0 removed this from the 0.6 milestone Mar 20, 2021
@msrd0 msrd0 removed a link to a pull request May 8, 2021
@msrd0
Copy link
Member

msrd0 commented May 8, 2021

This has been implemented for most use cases in #468. However, that pull request requires returned futures to live for 'static, which they won't do if you use the state inside an async block. This should be improved further.

@msrd0 msrd0 added this to the 0.7 milestone May 8, 2021
@msrd0 msrd0 removed this from the 0.7 milestone Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants