Opened 16 years ago

Closed 11 years ago

Last modified 11 years ago

#9084 closed Bug (fixed)

File-based session does not store any data on Windows

Reported by: mizutori Owned by: Joeri Bekker
Component: contrib.sessions Version: 1.5
Severity: Normal Keywords: sprint2013
Cc: wiser0+djangoproject@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Django 1.0 (or trunk r9029) has a bug that does not store any session data using file-based sessions on Windows system.


[1] Sample code

To use file-based sessions, set the SESSION_ENGINE setting to "django.contrib.sessions.backends.file". On Windows system, the following code does not work properly. The session data for "sess_key" will never be passed to the next index2 session.

---[ views.py ]---
def index(request):
    request.session['sess_key'] = "yes"
    request.session.modified = True
    return HttpResponseRedirect('index2.html')

def index2(request):
    print "session key="+repr(request.session['sess_key'])
    return HttpResponseRedirect('index3.html')
---

[2] My analysis

This is a bug of save() function of django.contrib.sessions.backends.file.SessionStore class. On Windows, os.rename function of Python will raise OSError if the destination file already exists. Whereas on Unix, it will be removed silently. To fix this problem, remove the existing file before renaming if the current operating system is Windows.


[3] My workaround

Apply unlink() function before rename() if the operating system is Windows to fix the save() function of file.SessionStore class.

---[ django/contrib/sessions/backends/file.py ]---
    def save(self, must_create=False):
        ...
        try:
            ...
#00122:
+           if os.name == "nt":
+               os.unlink(session_file_name)
            os.rename(output_file_name, session_file_name)
            renamed = True
---
# added patch code with "+" mark

Change History (24)

comment:1 by Malcolm Tredinnick, 16 years ago

Owner: changed from nobody to Malcolm Tredinnick
Status: newassigned
Triage Stage: UnreviewedAccepted

Whoops. Yes, you're right. os.rename() does need a clear landing zone on Windows. There's been talk from time to time of creating atomic_rename or something similar in Python that is cross-platform, but it doesn't exist yet.

comment:2 by Malcolm Tredinnick, 16 years ago

Summary: [BUG] file-based session does not store any data on WindowsFile-based session does not store any data on Windows

This is possibly going to be a bit tricky to solve. There are two race conditions that we need to worry about: firstly if another process comes in and reads the session file, the idea is that they see old data, not totally missing data. On windows, there's a small window in which another process will read between the unlink and the rename, which will be a bit confusing to the user.

Secondly, you can't unlink a file that's open in Windows, so there's the potential that the unlink will raise an error (again, because another process is coming in with the same session ID, perhaps due to multiple clicks of a browser button).

In both cases, it might be a case of tough luck -- we catch the error in the latter case and no save happens. But it'd be nice to avoid this if possible. Not sure how, though. Reintroducing locking there isn't going to be at all pleasant, since it's unbelievably tricky to get right in a portable fashion.

comment:3 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:4 by Malcolm Tredinnick, 15 years ago

Owner: Malcolm Tredinnick removed
Status: assignednew

I'm not going to have time to work on this for a bit and I want to clean out my list of "reserved" bugs. So giving it back to the pool. Thoughts in comment:2 are still valid, I feel. It's tricky, but I'm happy to review any patches that come along.

comment:5 by wiser0, 14 years ago

Cc: wiser0+djangoproject@… added
Version: 1.01.1

comment:6 by wiser0, 14 years ago

Has patch: set

comment:7 by anonymous, 14 years ago

Version: 1.11.2

This still doesn't work in django 1.2

comment:8 by Julien Phalip, 13 years ago

Patch needs improvement: set
Severity: Normal
Type: Bug

Patch needs improvement as per Malcolm's comments.

comment:9 by anonymous, 13 years ago

Easy pickings: unset
UI/UX: unset
Version: 1.21.3

comment:10 by Jannis Leidel, 13 years ago

Has patch: unset
Needs tests: set
Patch needs improvement: unset

Please provide a proper patch. This also needs tests.

comment:11 by anonymous, 11 years ago

This still happens in 1.4.1. Unfortunately it makes file-based sessions useless on Django in Windows (i.e., development or whatever). Also, it's hard to find, as the exception is caught without warning -- this wouldn't be so bad if it broke noisily.

comment:12 by Aymeric Augustin, 11 years ago

os.rename hasn't improved on Windows. Would it make sense to use shutil.copyfile instead when the platform is Windows?

This isn't atomic, and if I understand correctly, it may still be vulnerable to exceptions if the destination is open for reading at the wrong time.

But we can't do anything about that, besides telling people to use a better OS — ie. POSIX-compliant.

comment:13 by Joeri Bekker, 11 years ago

Keywords: sprint2013 added
Owner: set to Joeri Bekker
Status: newassigned

comment:14 by Joeri Bekker, 11 years ago

Has patch: set
Needs tests: unset
Triage Stage: AcceptedReady for checkin

I made a pull request https://github.com/django/django/pull/823 that changes os.rename to shutil.move which does (unlike the Python documentation indicates) an atomic os.rename but falls back to "the best Windows got to an atomic rename" in this specific case: shutil.copy2 and os.unlink.

The file based session save function is actually tested and fails on Windows:

(env) D:\Work\Projects\joeri\django\tests>python runtests.py --settings=test_sqlite sessions
Creating test database for alias 'default'...
Creating test database for alias 'other'...
........................................................................................x........................................................................................F................................
======================================================================
FAIL: test_clearsessions_command (django.contrib.sessions.tests.FileSessionTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\Work\Projects\joeri\django\django\test\utils.py", line 227, in inner
    return test_func(*args, **kwargs)
  File "D:\Work\Projects\joeri\django\django\contrib\sessions\tests.py", line 444, in test_clearsessions_command
    self.assertEqual(1, count_sessions())
AssertionError: 1 != 2

----------------------------------------------------------------------
Ran 210 tests in 0.276s

FAILED (failures=1, expected failures=1)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...

After patch:

(env) D:\Work\Projects\joeri\django\tests>python runtests.py --settings=test_sqlite sessions
Creating test database for alias 'default'...
Creating test database for alias 'other'...
........................................................................................x.........................................................................................................................
----------------------------------------------------------------------
Ran 210 tests in 0.235s

OK (expected failures=1)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...

comment:15 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In b9cc61021a0db1e5b41e61d3e53180e4fc618f9c:

Fixed #9084 - Best approach for an OS to atomically rename the session file.

comment:16 by spyjamesbond0072003@…, 11 years ago

Resolution: fixed
Status: closednew
Version: 1.31.5

Django 1.5.1 still not fix

comment:17 by Aymeric Augustin, 11 years ago

Resolution: fixed
Status: newclosed

It's fixed in master.

comment:18 by anonymous, 11 years ago

How to solve it? Django 1.5.1 still not fix

comment:19 by anonymous, 11 years ago

Confirmed, still not working in 1.5.1.

comment:20 by anonymous, 11 years ago

Resolution: fixed
Status: closednew

comment:21 by Simon Charette, 11 years ago

Resolution: fixed
Status: newclosed

The problem is fixed in master and 1.6a1.

It won't be backported to 1.5.X since it's not considered critical.

comment:22 by me21@…, 11 years ago

A new function is introduced in Python 3.3, os.replace(), which renames files atomically and cross-platformly: http://bugs.python.org/issue8828
It might be possible to use this function in future, when Django support for Python 2.x is abandoned, or when os.replace() is (ever) backported to Python 2.x.

comment:23 by me21@…, 11 years ago

In fact, since MoveFileEx API is considered to be atomic (for example, Java uses it: http://bugs.python.org/issue8828#msg146307), Django could use it through ctypes interface even now (https://github.com/mitsuhiko/python-atomicfile/ - _rename() function - and disregard that its author believes it to be non-atomic, it seems to be atomic actually). The os.replace() function in Python 3.3 is implemented through MoveFileEx anyway.

comment:24 by me21@…, 11 years ago

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top