Opened 7 years ago

Closed 2 years ago

Last modified 21 months ago

#9084 closed Bug (fixed)

File-based session does not store any data on Windows

Reported by: mizutori Owned by: joeri
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 Changed 7 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to mtredinnick
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 7 years ago by mtredinnick

  • Summary changed from [BUG] file-based session does not store any data on Windows to File-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 Changed 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:4 Changed 6 years ago by mtredinnick

  • Owner mtredinnick deleted
  • Status changed from assigned to new

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 Changed 6 years ago by wiser0

  • Cc wiser0+djangoproject@… added
  • Version changed from 1.0 to 1.1

comment:6 Changed 6 years ago by wiser0

  • Has patch set

comment:7 Changed 5 years ago by anonymous

  • Version changed from 1.1 to 1.2

This still doesn't work in django 1.2

comment:8 Changed 4 years ago by julien

  • Patch needs improvement set
  • Severity set to Normal
  • Type set to Bug

Patch needs improvement as per Malcolm's comments.

comment:9 Changed 4 years ago by anonymous

  • Easy pickings unset
  • UI/UX unset
  • Version changed from 1.2 to 1.3

comment:10 Changed 4 years ago by jezdez

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

Please provide a proper patch. This also needs tests.

comment:11 Changed 3 years ago by anonymous

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 Changed 3 years ago by aaugustin

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 Changed 2 years ago by joeri

  • Keywords sprint2013 added
  • Owner set to joeri
  • Status changed from new to assigned

comment:14 Changed 2 years ago by joeri

  • Has patch set
  • Needs tests unset
  • Triage Stage changed from Accepted to Ready 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 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

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

In b9cc61021a0db1e5b41e61d3e53180e4fc618f9c:

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

comment:16 Changed 2 years ago by spyjamesbond0072003@…

  • Resolution fixed deleted
  • Status changed from closed to new
  • Version changed from 1.3 to 1.5

Django 1.5.1 still not fix

comment:17 Changed 2 years ago by aaugustin

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

It's fixed in master.

comment:18 Changed 2 years ago by anonymous

How to solve it? Django 1.5.1 still not fix

comment:19 Changed 2 years ago by anonymous

Confirmed, still not working in 1.5.1.

comment:20 Changed 2 years ago by anonymous

  • Resolution fixed deleted
  • Status changed from closed to new

comment:21 Changed 2 years ago by charettes

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

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 Changed 2 years ago by me21@…

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 Changed 2 years ago by me21@…

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 Changed 21 months ago by me21@…

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