mem_cleanup throws "dictionary changed size during iteration"

• Key: MODPYTHON-57
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Minor
• Assignee: Nicolas Lehuen
• Reporter: Shack Toms
• Component/s: core
• Affects Version/s: 3.1.4
• Fix Version/s: 3.2.7

Description

The mem_cleanup routine, in Session.py, appears to have a bug that causes
python to throw "dictionary changed size during iteration".

The current code is.

def mem_cleanup(sdict):
    for sid in sdict:
        dict = sdict[sid]
        if (time.time() - dict["_accessed"]) > dict["_timeout"]:
            del sdict[sid]

The for statement should be changed to
    for sid in sdict.keys():

This will first generate a list of the keys of the sdict, and will avoid the
complaint about the dictionary changing on the "del sdict[sid]" statement.

The dbm_cleanup code has another approach, to use one iteration to gather
the keys to be deleted, and then have a second iteration over the gathered
keys which does the del. Using that approach, mem_cleanup should become...

def mem_cleanup(sdict):
    old = []
    for sid in sdict:
        dict = sdict[sid]
        if (time.time() - dict["_accessed"]) > dict["_timeout"]:
            old.append(sid)
    for sid in old:
        del sdict[sid]

This is more efficient if the number of sessions is large compared to the
number of sessions to be deleted.

Comments

We also have to protect against the case where some sessions are added to the sdict dictionary while the cleanup is being performed, or even against the case where two cleanups are performed concurrently. Hence, the same locking mechanisms that were used on DbmSession and FileSession should be applied here.

Fortunately we can rely on the GIL so that a call to sdict.keys() returns a snapshot of the SIDs at one point, so that even if some new session are added, we won't have a "dictionary changed size during iteration" exception, then protect against KeyError with a try..except block, in case another cleanup has deleted the session while we were examining it.

The final version is therefore :

def mem_cleanup(sdict):
    for sid in sdict.keys():
        try:
            session = sdict[sid]
            if (time.time() - session["_accessed"]) > session["_timeout"]:
                del sdict[sid]
        except:
            pass

This will do the trick, but a really clean implementation would rely on global locks, just like DbmSession and FileSession do.