Closed pagenoare closed 6 years ago
I'm fine with the idea, but it seems to me this approach has an issue.
The expiretime
is only updated because the session is saved back on access when the save_accessed_time
is True
. So if I set save_accessed_time=False
the expiretime will never be renewed and the session will expire after X seconds since it was created, not after X seconds since last time it was used (which is the behaviour beaker wants to provide).
Also we have another argument for sessions which is expire
and expire
forcefully requires save_accessed_time=True
exactly to prevent that issue.
So my suggestion would be use expire
itself instead of adding a new expiretime
argument. So that we avoid proliferation of options and we have in place checks for "save back session on read" that ensure the backend expiration is always renewed.
The backend expiration should probably always be a bit longer than the session expiration itself to prevent the case where the backend data expires while the session is being read (maybe add 1-2 minutes to it?)
Hi @amol- - I don't see expire
param. Did you mean timeout
? As indeed I can see piece of code that timeout
ensures save_accessed_time being True here:
if timeout and not save_accessed_time:
raise BeakerException("timeout requires save_accessed_time")
Thanks
Ooops, yeah, sorry, I meant timeout
. Got confused by the fact I was thinking of expiretime
.
I did some digging, and it seems timeout
param is not passed to Backend Namespace.
(Pdb) kw
(Pdb) {'digest_filenames': False, 'data_dir': '/home/vagrant/XXX/data/sessions', 'log_file': None}
I put timeout=X param into my middleware. If I use expiretime
param in middleware, it is passed further into Redis namespace. We could technically add that here:
data_dir=self.data_dir,
digest_filenames=False,
**self.namespace_args)
but I'm not sure if its something we want?
Apologizes if my questions are somewhat noob-ish, but I'm not very familiar with the beaker codebase yet.
Thanks!!
The options is captured and passed to the session through the middleware:
>>> from beaker.middleware import SessionMiddleware
>>> m = SessionMiddleware(None, config={'session.timeout': 10})
>>> s = m._get_session()
>>> s.timeout
10
But the Session
class itself intercepts it and doesn't let it propagate to namespace_args
(because it's not expected to be an option for the namespace manager).
So it should be modified to pass the timeout
option to namespace managers that support it, as not all namespace manager will support that option. Most of them (like file) won't be able to implement it and should not crash.
@amol- can you review whether my changes reflect your suggestions? Thanks much!
Hey,
I was trying to figure out what is the best way to clean up old / unused session data from Redis backend. I found that the basic mechanics for setting expiration time is there, but there's no way to use it from layers above. With that change, I'm able to just pass
expiretime
into Middleware, and voila - keys in reds are having expiration time set! :)Example usage of middleware:
Would be great if you could merge this change.
Thanks!
Kacper.