Opened 6 years ago

Closed 6 years ago

#3457 defect closed fixed (fixed)

Session expiry check frequency should be based on sessionTimeout

Reported by: mthuurne Owned by:
Priority: normal Milestone:
Component: web Keywords:
Cc: mwh Branch: branches/session-expiration-simplification-3457
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

Currently, there are two time intervals that play a role in session expiry: Site.sessionCheckTime and Session.sessionTimeout. In Twisted 2.5, the check time and timeout were hardcoded. Since Twisted 8.0, they are customizable. Ticket #1090 describes the actions taken to get rid of the hardcoded values.

The values of sessionCheckTime and sessionTimeout are not tied together in any way. This means that if you increase the sessionTimeout, session expiry is checked unnecessarily often. In the opposite case, sessions with a short sessionTimeout will not be cleaned up faster than the sessionCheckTime. Neither of this is a problem by itself: the default value of sessionCheckTime is 1800 and one check per 30 minutes is not likely to become a performance problem, while keeping expired sessions around for at most 30 minutes after they expired is also unlikely to consume a significant amount of memory.

However, it is possible to register a routine to be called when a session expires using Session.notifyOnExpire(). The current implementation will call the callback when the session is cleaned up, not when it expires. If for example sessionTimeout is set to 5 minutes and sessionCheckTime to 30 minutes, the expiry notification will be 25 minutes late. This could be a problem if for example you want to show a list of logged in users, since you will see users listed that did not log out, but are no longer logged in either because their session has expired.

The expiry check should be done as short as possible after the expiry time. One way of doing this would be to have a check frequency per Session instead of per Site, which is calculated from the sessionTimeout. Another way would be to use a kind of resettable watchdog timer instead of a periodic check: every time the session is used (Session.touch()), reset the timer to the timeout value. In either case, Site.sessionCheckTime would serve no function, so it could be deprecated or removed.

Change History (9)

comment:1 Changed 6 years ago by glyph

Thank you for a very clear description of the desired behavior :).

comment:2 Changed 6 years ago by exarkun

  • Owner changed from jknight to exarkun
  • Status changed from new to assigned

comment:3 Changed 6 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/session-expiration-simplification-3457

(In [26024]) Branching to 'session-expiration-simplification-3457'

comment:4 Changed 6 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted
  • Status changed from assigned to new

Indeed, there's no reason to have two different timeouts defined. I've tied expiration exactly to Session.sessionTimeout and deprecated the other stuff. Session expiration callbacks will now be called exactly when the session expires.

As this removes any period of time where a session has expired but is still present in the Site.sessions dictionary, this branch should resolved #3458 as well.

comment:5 follow-up: Changed 6 years ago by mwh

  • Cc mwh added
  • Keywords review removed
  • Owner set to exarkun
This is a Launchpad style comments-in-diff review, lucky you!  Or not.

I'm not an expert on this code by any means, so you might want to take
this review with a grain of salt, but I can see that this branch is a
nice simplification, and the new tests are nice.  A couple of comments
before it gets the nod though.

> Index: twisted/web/server.py
> ===================================================================
> --- twisted/web/server.py	(revision 26094)
> +++ twisted/web/server.py	(working copy)

> @@ -388,38 +389,48 @@
>      This utility class contains no functionality, but is used to
>      represent a session.
>
> +    @ivar _reactor: An object providing L{IReactorTime} to use for scheduling
> +        expiration.
>      @ivar sessionTimeout: timeout of a session, in seconds.
> -    @ivar loopFactory: factory for creating L{task.LoopingCall}. Mainly for
> -        testing.
> +    @ivar loopFactory: Deprecated in Twisted 9.0.  Does nothing.  Do not use.
>      """
>      sessionTimeout = 900
>      loopFactory = task.LoopingCall
>
> -    def __init__(self, site, uid):
> +    _expireCall = None
> +
> +    def __init__(self, site, uid, reactor=None):
>          """
>          Initialize a session with a unique ID for that session.
>          """
>          components.Componentized.__init__(self)
> +
> +        if reactor is None:
> +            from twisted.internet import reactor
> +        self._reactor = reactor
> +
>          self.site = site
>          self.uid = uid
>          self.expireCallbacks = []
> -        self.checkExpiredLoop = None
>          self.touch()
>          self.sessionNamespaces = {}
>
>
> -    def startCheckingExpiration(self, lifetime):
> +    def startCheckingExpiration(self, lifetime=None):
>          """
>          Start expiration tracking.
>
> -        @type lifetime: C{int} or C{float}
> -        @param lifetime: The number of seconds this session is allowed to be
> -        idle before it expires.
> +        @param lifetime: Ignored (deprecate this).

This sounds like a reminder to yourself.  It is already deprecated, though.

> @@ -464,10 +470,7 @@
>          If I haven't been touched in fifteen minutes, I will call my
>          expire method.
>          """
> -        # If I haven't been touched in 15 minutes:
> -        if self._getTime() - self.lastModified > self.sessionTimeout:
> -            if self.uid in self.site.sessions:
> -                self.expire()
> +        # Do nothing, deprecate this.

Why not deprecate it now?  This comment should probably go in the
docstring too.

> Index: twisted/web/test/test_web.py
> ===================================================================
> --- twisted/web/test/test_web.py	(revision 26094)
> +++ twisted/web/test/test_web.py	(working copy)
> @@ -11,6 +11,7 @@
>  from zope.interface.verify import verifyObject
>
>  from twisted.trial import unittest
> +from twisted.internet import reactor
>  from twisted.internet.address import IPv4Address
>  from twisted.internet.defer import Deferred
>  from twisted.web import server, resource, util
> @@ -233,76 +234,118 @@
>
>
>  class SessionTest(unittest.TestCase):
> +    """
> +    Tests for L{server.Session}.
> +    """
> +    def setUp(self):
> +        self.clock = task.Clock()
>
> -    def setUp(self):
> +        self.uid = 'unique'
> +        self.site = server.Site(resource.Resource())
> +        self.session = server.Session(self.site, self.uid, self.clock)
> +        self.site.sessions['unique'] = self.session

This line should probably read:

    self.site.sessions[self.uid] = self.session

I think a docstring would be nice too, something like

    """
    Set up a site and a session for it that has a controllable
    notion of time.
    """

(but feel free to fiddle with the wording)

> +
> +    def test_defaultReactor(self):
>          """
> -        Set up a session using a simulated scheduler. Creates a
> -        C{times} attribute which specifies the return values of the
> -        session's C{_getTime} method.
> +        If not value is passed to L{server.Session.__init__}, the global
> +        reactor is used.
>          """
> -        clock = self.clock = task.Clock()
> -        times = self.times = []
> +        session = server.Session(server.Site(resource.Resource()), '123')
> +        self.assertIdentical(session._reactor, reactor)
>
> -        class MockSession(server.Session):
> -            """
> -            A mock L{server.Session} object which fakes out scheduling
> -            with the C{clock} attribute and fakes out the current time
> -            to be the elements of L{SessionTest}'s C{times} attribute.
> -            """
> -            def loopFactory(self, *a, **kw):
> -                """
> -                Create a L{task.LoopingCall} which uses
> -                L{SessionTest}'s C{clock} attribute.
> -                """
> -                call = task.LoopingCall(*a, **kw)
> -                call.clock = clock
> -                return call
>
> -            def _getTime(self):
> -                return times.pop(0)
> +    def test_startCheckingExpiration(self):
> +        """
> +        L{server.Session.startCheckingExpiration} causes the session to expire
> +        after L{server.Session.sessionTimeout} seconds without activity.
> +        """
> +        self.session.startCheckingExpiration()
>
> -        self.site = server.Site(SimpleResource())
> -        self.site.sessionFactory = MockSession
> +        # Advance to almost the timeout - nothing should happen.
> +        self.clock.advance(self.session.sessionTimeout - 1)
> +        self.assertIn(self.uid, self.site.sessions)
>
> +        # Advance to the timeout, the session should expire.
> +        self.clock.advance(1)

Maybe I'm a wuss but I always get sensitive about testing what happens
when you hit timeouts right on the nose like this.  If it's specified
somewhere, fine, but I would find this easier to be confident if you
advanced past the timeout, i.e. said self.clock.advance(2).

> +        self.assertNotIn(self.uid, self.site.sessions)
>
> -    def test_basicExpiration(self):
> +        # There should be no calls left over, either.
> +        self.assertFalse(self.clock.calls)
> +
> +
> +    def test_expire(self):
>          """
> -        Test session expiration: setup a session, and simulate an expiration
> -        time.
> +        L{server.Session.expire} expires the session.
>          """
> -        self.times.extend([0, server.Session.sessionTimeout + 1])
> -        session = self.site.makeSession()
> -        hasExpired = [False]
> -        def cbExpire():
> -            hasExpired[0] = True
> -        session.notifyOnExpire(cbExpire)
> -        self.clock.advance(server.Site.sessionCheckTime - 1)
> -        # Looping call should not have been executed
> -        self.failIf(hasExpired[0])
> +        self.session.expire()
> +        # It should be gone from the session dictionary.
> +        self.assertNotIn(self.uid, self.site.sessions)
> +        # And there should be no pending delayed calls.
> +        self.assertFalse(self.clock.calls)
>
> -        self.clock.advance(1)
>
> -        self.failUnless(hasExpired[0])
> +    def test_expireWhileChecking(self):
> +        """
> +        L{server.Session.expire} expires the session even if the timeout call
> +        isn't due yet.
> +        """
> +        self.session.startCheckingExpiration()
> +        self.test_expire()

This (calling a test method from another) is an anti-pattern in xunit
test patterns, but I forget why, so never mind :)

comment:6 in reply to: ↑ 5 Changed 6 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

Replying to mwh:

This is a Launchpad style comments-in-diff review, lucky you! Or not.

Hooray, exchanges with foreign cultures.

[snip]

> > +    def startCheckingExpiration(self, lifetime=None):
> >          """
> >          Start expiration tracking.
> >
> > -        @type lifetime: C{int} or C{float}
> > -        @param lifetime: The number of seconds this session is allowed to be
> > -        idle before it expires.
> > +        @param lifetime: Ignored (deprecate this).

This sounds like a reminder to yourself. It is already deprecated, though.

Yep. I fixed the docstring.

> > @@ -464,10 +470,7 @@
> >          If I haven't been touched in fifteen minutes, I will call my
> >          expire method.
> >          """
> > -        # If I haven't been touched in 15 minutes:
> > -        if self._getTime() - self.lastModified > self.sessionTimeout:
> > -            if self.uid in self.site.sessions:
> > -                self.expire()
> > +        # Do nothing, deprecate this.

Why not deprecate it now? This comment should probably go in the
docstring too.

Indeed. I deprecated it, added a test, and changed the docstring.

[snip]

> > -    def setUp(self):
> > +        self.uid = 'unique'
> > +        self.site = server.Site(resource.Resource())
> > +        self.session = server.Session(self.site, self.uid, self.clock)
> > +        self.site.sessions['unique'] = self.session
>

This line should probably read:

self.site.sessions[self.uid] = self.session

I think a docstring would be nice too, something like

[snip]

Fixed both issues.

[snip]

Maybe I'm a wuss but I always get sensitive about testing what happens
when you hit timeouts right on the nose like this. If it's specified
somewhere, fine, but I would find this easier to be confident if you
advanced past the timeout, i.e. said self.clock.advance(2).

I feel okay about this one because there is a "positive assertion" afterward - something which can only happen if the timed call actually ran. I also don't think Clock is going to change to required N+epsilon to elapse before a timed call is run, rather than just N. I like to hit the boundaries exactly, when possible, to make sure an off-by-one hasn't crept into the implementation somehow.

[snip]

> > +    def test_expireWhileChecking(self):
> > +        """
> > +        L{server.Session.expire} expires the session even if the timeout call
> > +        isn't due yet.
> > +        """
> > +        self.session.startCheckingExpiration()
> > +        self.test_expire()

This (calling a test method from another) is an anti-pattern in xunit
test patterns, but I forget why, so never mind :)

Yea, I don't feel great about it, but it hasn't bit me yet - at least not any worse than any other kind of code sharing in unit tests. I'm trying not to make a habit of this, but I'm inclined not to worry about one or two cases.

comment:7 Changed 6 years ago by mwh

  • Keywords review removed
  • Owner set to exarkun

NIce work, r=me -- oops more launchpad styleee. Please land :)

comment:8 Changed 6 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [26133]) Merge session-expiration-simplification-3457

Author: exarkun
Reviewer: mwhudson
Fixes: #3457

Replace the complicated two-stage session timeout logic from Twisted Web and
replace it with a single stage timeout mechanism (where sessions time out after
N seconds of inactivity).

comment:9 Changed 4 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.