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

Cannot modify session expiry #36

Open
mdtusz opened this issue Jun 24, 2022 · 4 comments
Open

Cannot modify session expiry #36

mdtusz opened this issue Jun 24, 2022 · 4 comments

Comments

@mdtusz
Copy link

mdtusz commented Jun 24, 2022

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 an Arc (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:

  1. the expiry not being wrapped in Arc so the cloned session (via tide session middleware's handler) does not update the session expiry
  2. Session::expire_in does not mark the session data_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 fixed

These 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.

@jbr
Copy link
Member

jbr commented Jun 24, 2022

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

@mdtusz
Copy link
Author

mdtusz commented Jun 24, 2022

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 expire_in intended because it will now work.

There's unfortunately no workaround I can think of for this other than to modify the Session struct as well (barring a larger change to the tide::SessionMiddleware).

@jbr
Copy link
Member

jbr commented Jun 24, 2022

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 Arc<RwLock<_>> in tide's middleware might be the simplest way to fix this (and removing the interior mutability).

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.

@mdtusz
Copy link
Author

mdtusz commented Jun 24, 2022

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.

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

No branches or pull requests

2 participants