search

taoensso / carmine

Redis client + message queue for Clojure
https://www.taoensso.com/carmine
Eclipse Public License 1.0
1.15k stars 130 forks source link

Ring middleware: expiration is not reset when session key is read #259

Closed svdo closed 1 year ago

svdo commented 2 years ago

Steps to reproduce:

  1. Create a project using ring and this carmine ring session middleware
  2. Configure the carmine store to have a session expiration of 10 seconds
  3. Login and verify in Redis monitor that the session is created
  4. Wait five seconds and refresh the browser; in Redis monitor you can see that the keys is read
  5. Wait six seconds and refresh the browser

Expected behavior: session is still active because only six seconds have passed after the last request

Actual behavior: session has expired because more than 10 seconds have passed after the session was created.

See PR #254.

ptaoussanis commented 2 years ago

@svdo Hi Stefan, thanks for this!

The current behaviour is intentional though, and documented here.

There's cases where one might want to refresh session on write only, and cases where one might want to refresh on reads too.

Your proposed change would break the documented behaviour, so isn't an option. But would be open to see a PR that extends carmine-store to add a new :extend-on-read? option (default false).

Hope that makes sense!

Cheers :-)

svdo commented 2 years ago

Thanks for your quick response @ptaoussanis! Thanks for pointing out the backward compatibility issue, I agree that this should be behind an option. I'll modify the PR accordingly.

Do you mind about the inclusion of deps.edn that I did?

ptaoussanis commented 2 years ago

Do you mind about the inclusion of deps.edn that I did?

Would prefer that be a separate PR, thanks!

svdo commented 2 years ago

@ptaoussanis I have created as new pull request. Please let me know if this needs any more work!

ptaoussanis commented 1 year ago

PR merged, closing. Thanks again!