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

Fix #762 #769

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Fix #762 #769

wants to merge 6 commits into from

Conversation

pepoviola
Copy link
Contributor

@pepoviola pepoviola commented Dec 26, 2020

Hi @yoshuawuyts / @No9,
I make this changes trying to fix the bug reported in #762, where the cookie id isn't updated ( resent ) when the session is regenerated. I put the session behind an Arc::RwLock ( std ) when we set the ext and then in the request return the guard to don't change the req api. ( e.g req.session() / req.session_mut() ). It's ok to use the RwLock from std::sync ?

Also, I wonder if we still need the RwLock and the Arc in the Session struct ( https://github.com/http-rs/async-session/blob/main/src/session.rs#L60-L67 ) if we already are using a mutex ( from this pr ) on top to of that struct ?

Open here as draft since I'm not sure if my approach is ok, and the code have this clippy warnings that I'm not sure how to resolve.

519 |     pub fn session_mut(&mut self) -> std::sync::RwLockWriteGuard<crate::sessions::Session> {
    |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: indicate the anonymous lifetime: `std::sync::RwLockWriteGuard<'_, crate::sessions::Session>`

Thanks! any feedback is welcome :)

@jbr
Copy link
Member

jbr commented Dec 26, 2020

I'd be happy to talk through the design considerations that went into the current approach. It's an intentional aspect of the design that we drop the cookie value on clone

@pepoviola
Copy link
Contributor Author

pepoviola commented Dec 26, 2020

Hi @jbr, thanks for the reply. Yes, that would we great! As I say I'm not sure if the fix is in the correct path, I also realize that the test in async-session expect that the cookie gets dropped.

Thanks for the feedback!🙌

Update: I change the code to extract the value from the Arc::RwLock and don't need to clone. Also I closed http-rs/async-session#16 because cloning the cookie values is no more necessary.
Thx!

@pepoviola
Copy link
Contributor Author

Hi @jbr, If you have time can we make a quick talk to understand the design and how to resolve the regenerate issue. Maybe is better to have an struct Session that holds an inner struct with a Arc but i'm not sure.

Thanks in advance!

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 this pull request may close these issues.

2 participants