fix get_session_remaining_seconds() for rolling sessions #70
Conversation
deepcyrille
force-pushed
the
fix_rolling_sessions_remaining_seconds
branch
2 times, most recently
from
65b13d1
to
1b5f783
Compare
`get_session_remaining_seconds()` should return `last_update + lifetime - now` instead of `created + lifetime - now`. Fix mypy: use FixtureRequest instead of SubRequest `SubRequest` is a subclass of `FixtureRequest`, but is currently private so pytest-asyncio uses `Any` instead. However, `FixtureRequest` typing is sufficient for our needs, so can use that instead.
deepcyrille
force-pushed
the
fix_rolling_sessions_remaining_seconds
branch
from
1b5f783
to
0ff538a
Compare
| return int((metadata["created"] + metadata["lifetime"]) - now) | ||
| # use "last_update" if rolling session, otherwise use "created" | ||
| rolling = metadata.get("rolling", False) | ||
| last_update = metadata["last_update"] if rolling and "last_update" in metadata else metadata["created"] |
There was a problem hiding this comment.
What if we will use last_access everywhere? This seems very natural to me. Rolling session is alive while it's being used. So last_access may work.
There was a problem hiding this comment.
I tried that but last_access is updated before in the load function, whereas last_update is updated in the save function
There was a problem hiding this comment.
I believe we should use last_access here, as the purpose of rolling session is not to expire while being used.
last_access is the timestamp when the session was last loaded, read used.
I think that we should not use last_updated to count session remaining time because I think it is a good idea not to call handler.save at all if session was not modified to avoid network round trip, e.g. with Redis backend (feature out of scope).
Since we always update last_access on every load (read endpoint call), we do extend rolling session TTL by this action. So, my opinion is just to use last_access in place of created and this should solve your requirement.
|
Anything I should do to get this PR released? |
|
I will get back to it in 2-3 days. |
get_session_remaining_seconds()should returnlast_update + lifetime - nowinstead ofcreated + lifetime - now.