Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#21034 closed Bug (fixed)

atomic.__exit__ should check exc_type rather than exc_value

Reported by: Aymeric Augustin Owned by: Aymeric Augustin
Component: Database layer (models, ORM) Version: 1.6-beta-1
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (9)

comment:1 by Christophe Pettus, 11 years ago

It's fixed in xact now, thanks!

comment:2 by Aymeric Augustin, 11 years ago

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.

Last edited 11 years ago by Aymeric Augustin (previous) (diff)

comment:4 by Thomas C, 11 years ago

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

comment:5 by Aymeric Augustin, 11 years ago

That works on Python 2.6 but not on Python 2.7.

comment:6 by Aymeric Augustin, 11 years ago

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.

comment:7 by Aymeric Augustin, 11 years ago

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

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

Resolution: fixed
Status: newclosed

In 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.

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

In 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.

Fixed #21034.

Forward-port of a8624b2 from 1.6.x.

Note: See TracTickets for help on using tickets.
Back to Top