atomic.__exit__ should check exc_type rather than exc_value

[Aymeric Augustin]

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.

[Christophe Pettus]

It's fixed in xact now, thanks!

[Aymeric Augustin]

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.

[Aymeric Augustin]

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

[Thomas C]

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

[Aymeric Augustin]

That works on Python 2.6 but not on Python 2.7.

[Aymeric Augustin]

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.

[Aymeric Augustin]

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

[Aymeric Augustin <aymeric.augustin@…>]

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.

[Aymeric Augustin <aymeric.augustin@…>]

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.