Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#21034 closed Bug (fixed)

atomic.__exit__ should check exc_type rather than exc_value

Reported by: aaugustin Owned by: aaugustin
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 Changed 2 years ago by Xof

It's fixed in xact now, thanks!

comment:2 Changed 2 years ago 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.

Last edited 2 years ago by aaugustin (previous) (diff)

comment:4 Changed 2 years ago 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

comment:5 Changed 2 years ago by aaugustin

That works on Python 2.6 but not on Python 2.7.

comment:6 Changed 2 years ago 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.

comment:7 Changed 2 years ago by aaugustin

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

comment:8 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

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

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

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