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

[ring middleware] Reset session expiration on every read #261

Closed
wants to merge 4 commits into from
Closed

[ring middleware] Reset session expiration on every read #261

wants to merge 4 commits into from

Conversation

svdo
Copy link
Contributor

@svdo svdo commented Feb 4, 2022

This change introduces a new option :extend-on-read? to the Ring middleware. By default it is false. When false, the session expiration will be set when it is created (and not changed afterwards) and when it is written (which normally doesn't happen when using the ring-session middleware). When true the session expiration is reset on every read, so that the session is always valid for the set number of seconds after it was last used.

Updated pull request for #259.

@ptaoussanis ptaoussanis self-assigned this Feb 5, 2022
@ptaoussanis
Copy link
Member

ptaoussanis commented Feb 5, 2022

@svdo Thanks Stefan! Will look at merging next time I'm doing some batched work on Carmine.

Just a heads-up that this part of your understanding isn't accurate:

When false, the session expiration will be set when it is created (and not changed afterwards)

When false, the session is also renewed on any session writes (changes).

I.e. the extend-on-read? options would be:

  • false (default): renew session only on session writes
  • true: renew session also on session reads

Hope that makes sense!

@svdo
Copy link
Contributor Author

svdo commented Feb 5, 2022

Yes I understand, thanks for clarifying!!

@ptaoussanis ptaoussanis force-pushed the master branch 4 times, most recently from 5ebd72d to e483550 Compare November 1, 2022 13:45
@ptaoussanis ptaoussanis force-pushed the master branch 4 times, most recently from 2e00f40 to c9ed46c Compare December 2, 2022 16:54
@ptaoussanis
Copy link
Member

@svdo Will be merged in next release, thanks Stefan! Apologies for the long delay.

@svdo
Copy link
Contributor Author

svdo commented Dec 3, 2022

No problem at all @ptaoussanis, I've been using my fork so that didn't cause any issues for me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants