-
Notifications
You must be signed in to change notification settings - Fork 21
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
Cannot modify session expiry #36
Comments
I think this would require a semver-breaking release of async-session, but it's been a while since there's been a tide release and tide is already out of date from async-session. Tide's current development status is described here |
Definitely seems like it would warrant a semver-major release. I'm happy to make the changes for this if it's agreed upon that it's a change worth making. After a bit deeper thinking though, I'm not sure that this would cause too many in-the-wild issues - if someone had been modifying the session expiry as of now, nothing would have been happening and they would surely take notice that their intended effect wasn't working. Still a breaking change, but the changelog notice should basically be to check that any usage of There's unfortunately no workaround I can think of for this other than to modify the |
As far as larger changes: There's a larger change I've been wanting to make that removes all interior mutability from Session, but that might be a later change. The interior mutability of some of the fields (but not all of them) is a consequence of the fact that async-session was built for tide, and had to work around tide's inability to pass any owned data through the request-response cycle (since request extensions are dropped along with the request). I feel like the behavior (bug or misfeature) that this issue is about is a consequence of that design. Now that a number of other frameworks use async-session, it makes more sense to move any tide-specific workarounds into the tide middleware and out of async-session. Wrapping the entire Session in an Regardless of whether we treat this as a semver-patch or semver-minor release of async-session, tide would require a new release because tide's already out of date, and has been for a while. I'll take a look at making the above changes, but my guess is that it's going to be a slow road to a new tide release. Probably the best choice for tide would be publishing a standalone middleware as an external crate that can be used instead of the built-in middleware, just like I did with driftwood as a replacement for the tide default logger. |
Let me know if I can help in tackling anything on this. We're using tide so have a vested interest in seeing it's development stay active and alive (and I don't want to switch to axum...) so I'm happy to jump in and help more often with tide (and associated Fishrock/Yoshua/Jbr tide-related projects) maintenance. |
It seems when using the
Session
with tide's session middleware, it's not possible to modify the session expiry because it is not wrapped in anArc
(I think).This can be a somewhat tricky thing to reason with because of the session lifecycle in a request (and how it is cloned), but testing reveals that there are two things that would prevent this:
Arc
so the cloned session (via tide session middleware's handler) does not update the session expirySession::expire_in
does not mark the sessiondata_changed
, so unless the check is disabled in the middleware, changes to session expiry will not cause the session to be updated by the store even it 1 is fixedThese are relatively straightforwards things to resolve, but may have some more subtle implications since it will change how sessions behave and existing code may be out there which depends on these quirks.
The text was updated successfully, but these errors were encountered: