[Django] #21034: atomic.__exit__ should check exc_type rather than exc

Django

unread,

Sep 4, 2013, 1:46:37 PM9/4/13

to django-...@googlegroups.com

#21034: atomic.__exit__ should check exc_type rather than exc_value
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: aaugustin
Type: Bug | Status: new
Component: Database | Version: 1.6-beta-1
layer (models, ORM) | Keywords:
Severity: Release | Has patch: 0
blocker | Needs tests: 0
Triage Stage: Accepted | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
This bug was reported by Thomas Chaumeny after we hit it with our fork of
`xact`, which `atomic` derives from.

Under Python 2, in a context manager's `__exit__`, `exc_value` may be
`None` even though an exception occured and `exc_type` isn't `None`.

Django currently checks if `exc_value is None` to determine if an
exception occured. It should check if `exc_type is None` instead.

Proof:

{{{
% python2.6

>>> class context_manager:
... def __enter__(self):
... pass
... def __exit__(self, type, value, traceback):
... print('type: %s %s' % (type, type is None))
... print('value: %s %s' % (value, value is None))

>>> with context_manager():
... raise ValueError('foo')
...
type: <type 'exceptions.ValueError'> False
value: foo False
Traceback (most recent call last):
File "<stdin>", line 2, in <module>
ValueError: foo

>>> with context_manager():
... raise KeyboardInterrupt
...
type: <type 'exceptions.KeyboardInterrupt'> False
value: False
Traceback (most recent call last):
File "<stdin>", line 2, in <module>
KeyboardInterrupt

>>> with context_manager():
... import time; time.sleep(10)
...
^Ctype: <type 'exceptions.KeyboardInterrupt'> False
value: None True
Traceback (most recent call last):
File "<stdin>", line 2, in <module>
KeyboardInterrupt
}}}

This bug doesn't exist in Python 3:

{{{
% python3.3

>>> class context_manager:
... def __enter__(self):
... pass
... def __exit__(self, type, value, traceback):
... print('type: {} {}'.format(type, type is None))
... print('value: {} {}'.format(value, value is None))
...

>>> with context_manager():
... raise ValueError('foo')
...
type: <class 'ValueError'> False
value: foo False
Traceback (most recent call last):
File "<stdin>", line 2, in <module>
ValueError: foo

>>> with context_manager():
... raise KeyboardInterrupt
...
type: <class 'KeyboardInterrupt'> False
value: False
Traceback (most recent call last):
File "<stdin>", line 2, in <module>
KeyboardInterrupt

>>> with context_manager():
... import time; time.sleep(10)
...
^Ctype: <class 'KeyboardInterrupt'> False
value: False
Traceback (most recent call last):
File "<stdin>", line 2, in <module>
KeyboardInterrupt
}}}

This is a release blocker because it's a major bug in a new feature. It
might lead to data loss.

--
Ticket URL: <https://code.djangoproject.com/ticket/21034>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,

Sep 4, 2013, 1:51:18 PM9/4/13

to django-...@googlegroups.com

#21034: atomic.__exit__ should check exc_type rather than exc_value
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: aaugustin
Type: Bug | Status: new

Comment (by Xof):

It's fixed in xact now, thanks!

--
Ticket URL: <https://code.djangoproject.com/ticket/21034#comment:1>

Django

unread,

Sep 4, 2013, 1:51:24 PM9/4/13

to django-...@googlegroups.com

#21034: atomic.__exit__ should check exc_type rather than exc_value
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: aaugustin
Type: Bug | Status: new

Comment (by aaugustin):

It's going to be extremely hard to write a test case for this: as
demonstrated above, hitting ^C on the command line triggers the bug, but
raising KeyboardInterrupt in Python code doesn't. I'm tempted to just
commit the fix.

Does someone have an idea to test this?

----

I'm also going to audit other context managers for the same problem.

--
Ticket URL: <https://code.djangoproject.com/ticket/21034#comment:2>

Django

unread,

Sep 4, 2013, 1:51:38 PM9/4/13

to django-...@googlegroups.com

#21034: atomic.__exit__ should check exc_type rather than exc_value
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: aaugustin
Type: Bug | Status: new

Comment (by aaugustin):

Fix in xact:
https://github.com/Xof/xact/commit/18ea837c6ff8ef41d236bd2569eedc8093532dfd

--
Ticket URL: <https://code.djangoproject.com/ticket/21034#comment:3>

Django

unread,

Sep 4, 2013, 4:34:12 PM9/4/13

to django-...@googlegroups.com

#21034: atomic.__exit__ should check exc_type rather than exc_value
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: aaugustin
Type: Bug | Status: new

Comment (by tchaumeny):

You could test this by calling sys.exit() inside a try/except block.
It will raise an exception with None value as shown below (Python 2.6).

{{{
In [1]: class Foo:

...: def __enter__(self):
...: pass

...: def __exit__(self, exc_type, exc_val, trb):
...: print exc_val is None
...:

In [2]: try:
...: with Foo():
...: import sys; sys.exit()
...: except:
...: pass
...:
True
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/21034#comment:4>

Django

unread,

Sep 4, 2013, 5:07:45 PM9/4/13

to django-...@googlegroups.com

#21034: atomic.__exit__ should check exc_type rather than exc_value
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: aaugustin
Type: Bug | Status: new

Comment (by aaugustin):

That works on Python 2.6 but not on Python 2.7.

--
Ticket URL: <https://code.djangoproject.com/ticket/21034#comment:5>

Django

unread,

Sep 4, 2013, 5:14:36 PM9/4/13

to django-...@googlegroups.com

#21034: atomic.__exit__ should check exc_type rather than exc_value
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: aaugustin
Type: Bug | Status: new

Comment (by aaugustin):

I don't have a way to trigger this edge case on Python 2.7, the lowest
version supported by Django 1.7. To be honest I don't even know if it's
possible! Therefore I won't include a test — there's no point in including
a test that passes before and after the change.

The same bug exists in the old, deprecated transaction management code.
Fixing it would require extensive refactoring. I'm leaving it alone. That
code is going away anyway.

All other context managers properly test for `exc_type is None`.

--
Ticket URL: <https://code.djangoproject.com/ticket/21034#comment:6>

Django

unread,

Sep 4, 2013, 5:16:02 PM9/4/13

to django-...@googlegroups.com

#21034: atomic.__exit__ should check exc_type rather than exc_value
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: aaugustin
Type: Bug | Status: new

Comment (by aaugustin):

Scratch that -- it's actually posssible to fix it in the deprecated
transaction management too.

--
Ticket URL: <https://code.djangoproject.com/ticket/21034#comment:7>

Django

unread,

Sep 4, 2013, 5:19:37 PM9/4/13

to django-...@googlegroups.com

#21034: atomic.__exit__ should check exc_type rather than exc_value
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: aaugustin

Type: Bug | Status: closed

Component: Database layer | Version:
(models, ORM) | 1.6-beta-1

Severity: Release blocker | Resolution: fixed

Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Aymeric Augustin <aymeric.augustin@…>):

* status: new => closed
* resolution: => fixed

Comment:

In [changeset:"a8624b22a786f332d535d77ce51b97bde5b3e05e"]:
{{{
#!CommitTicketReference repository=""
revision="a8624b22a786f332d535d77ce51b97bde5b3e05e"
[1.6.x] Tested exc_type instead of exc_value in __exit__.

exc_value might be None even though there's an exception, at least on
Python 2.6. Thanks Thomas Chaumeny for the report.

Fixed #21034.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/21034#comment:8>

Django

unread,

Sep 4, 2013, 5:19:37 PM9/4/13

to django-...@googlegroups.com

#21034: atomic.__exit__ should check exc_type rather than exc_value
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: aaugustin

Comment (by Aymeric Augustin <aymeric.augustin@…>):

In [changeset:"4170b9f402efa7c3c065b02da32555fca2c631c0"]:
{{{
#!CommitTicketReference repository=""
revision="4170b9f402efa7c3c065b02da32555fca2c631c0"

Tested exc_type instead of exc_value in __exit__.

exc_value might be None even though there's an exception, at least on
Python 2.6. Thanks Thomas Chaumeny for the report.

[Django] #21034: atomic.__exit__ should check exc_type rather than exc_value

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

[Django] #21034: atomic.exit should check exc_type rather than exc_value