sessions: Race condition in FilesystemStore

What is the situation on this?

“we could provide a default implementation using in-memory locks”

This would be meaningless in a multi-server environment where requests are load balanced.

There’s a good article about the nuances of per-session locking here:

http://thwartedefforts.org/2006/11/11/race-conditions-with-ajax-and-php-sessions/

As we’ve already concluded here the timeout for locks if something goes wrong is one of the major issues implementing this kind of scheme. This needs more thought and is very backend dependant

As far as implementing Lock/Release we could make the per-session lock implementation optional by having a separate interface. The backend could then be asserted against that interface and if it does not implement it we could provide a default implementation using in-memory locks.

@cless Good points.

I like your proposed interface changes.

Your lock expiration would be relatively easy to implement for RediStore, Redis has an EXPIRE command which allows a TTL to be set for a key.

here are some real world examples of the impact of session race conditions. The bugs are hard to debug, show up seemingly at random and are an annoyance for both developers and users. Given a sufficiently large application that makes extensive use of ajax these race conditions will show up sooner or later. http://www.hiretheworld.com/blog/tech-blog/codeigniter-session-race-conditions http://www.chipmunkninja.com/Troubles-with-Asynchronous-Ajax-Requests-g@

I’ve read the whole thread at EllisLab/CodeIgniter#1746 which deals with a similar issue, but their issue is complicated by the fact that CodeIgniter forcfully regenerates the session id every once in a while and most of the discussion there revolves around this fact.

I understand your reluctance to change sessions in an incompatible way or to introduce subtle differences in the API that might break existing applications. However, I still think there should be some optional locking involved. What about adding two functions to the store API like this:

type Store interface {
    Get(r *http.Request, name string) (*Session, error)
    New(r *http.Request, name string) (*Session, error)
    Save(r *http.Request, w http.ResponseWriter, s *Session) error
    Lock(r *http.Request, name string) error
    Release(r *http.Request, name string) error
}

Use of these functions would be entirely optional (although I would personally encourage locking in every request that is going to write to the session) and have no impact on existing applications.

There is an issue with the proposed locking interface: if the lock is held in a database such as MySQL and your database connection drops after you acquire the lock then you can never release it and your session is effectively deadlocked. Locks should expire in some way, but I am not entirely sure how that should be exposed to the developer.

The default Django session backend is the database and this uses the Django database abstraction layer which I am unfamiliar with so it is hard for me to interpret. However, I found this comment in the filesystem backend:

        # Write the session file without interfering with other threads
        # or processes.  By writing to an atomically generated temporary
        # file and then using the atomic os.rename() to make the complete
        # file visible, we avoid having to lock the session file, while
        # still maintaining its integrity.
        #
        # Note: Locking the session file was explored, but rejected in part
        # because in order to be atomic and cross-platform, it required a
        # long-lived lock file for each session, doubling the number of
        # files in the session storage directory at any given time.  This
        # rename solution is cleaner and avoids any additional overhead
        # when reading the session data, which is the more common case
        # unless SESSION_SAVE_EVERY_REQUEST = True.
        #
        # See ticket #8616.

This seems to suggest they make sure the session file contents are never mangled but not that no race conditions can happen. If anyone has Django experience it would be nice to see a test case like cless/sessions@f84abed Stackoverflow also seems to indicate Django might have race conditions in the sessions: http://stackoverflow.com/search?q=django+session+race+condition