Opened 5 years ago

Closed 10 days ago

#16682 closed Bug (fixed)

KeyboardInterrupt not handled properly in transaction aborting

Reported by: Malcolm Tredinnick Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Ticket #6928 was reopened to report this, so I (Malcolm) am moving it to a new ticket, since it's a separate issue. Following text is from comment:7 on that ticket:

---

After [14288] transaction.commit_on_success does not handle KeyboardInterrupt.

Problem in django.db.transaction.Transaction.__exit__ and default exiting function.

    def __exit__(self, exc_type, exc_value, traceback):
        self.exiting(exc_value, self.using)

exc_value is None after KeyboardInterrupt has been throwed, only exc_type and traceback has value.
But if raise KeyboardInterrupt manualy from code it's work ok.

Possible solution: use exc_type and exc_value in exiting function

I use python 2.6.6

Attachments (2)

16682-test.diff (1.1 KB) - added by Claude Paroz 5 years ago.
Failing test
16682-atomic.diff (1.0 KB) - added by Tim Graham 11 days ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 5 years ago by Aymeric Augustin

Resolution: needsinfo
Status: newclosed

I'm sorry, but this report doesn't contain enough information to reproduce the problem. How can you get a KeyboardInterrupt within transaction.commit_on_success?

I wrote this test code:

# models.py

from django.db import models

class FooModel(models.Model):
    foo = models.CharField(max_length=42)

# views.py

import time
from django.db import transaction
from .models import FooModel

@transaction.commit_on_success
def interrupt_me(request):
    FooModel.objects.create(foo='foo')
    print "Hit Ctrl-C now!"
    time.sleep(5)
    print "Too late, and I didn't bother to return a HttpResponse"

and hooked the view in the URLconf.

Then I ran: ./manage.py runserver --traceback

I open the URL in a browser, and during the sleep, I hit Ctrl-C in the console. runserver exits cleanly, with or without transaction.commit_on_success.

Please provide a test case or instructions to reproduce your problem and re-open the ticket.

comment:4 Changed 5 years ago by Claude Paroz

Resolution: needsinfo
Status: closedreopened
Triage Stage: UnreviewedAccepted

I've been able to reproduce it with the following code (you can insert it in regressiontests/transactions_regress/tests.py):

    def test_rollback_on_keyboardinterrupt(self):
        import time
        try:
            with transaction.commit_on_success():
                Mod.objects.create(fld=17624)
                print "Hit Ctrl-C now!"
                time.sleep(5)
                print "Too late, and I didn't bother to return a HttpResponse"
        except KeyboardInterrupt:
            pass
        self.assertEqual(Mod.objects.all().count(), 0)

Unfortunately, I didn't manage to simulate a Ctrl-C programmatically, so as to be able to include a real test. Ideas welcome.

comment:5 Changed 5 years ago by Claude Paroz

#17624 is a duplicate with a patch

Changed 5 years ago by Claude Paroz

Attachment: 16682-test.diff added

Failing test

comment:6 Changed 5 years ago by Claude Paroz

I eventually found a trick to generate the KeyboardInterrupt to create a failing test. Don't ask me why the os.kill has to be called twice...

comment:7 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:8 Changed 11 days ago by Tim Graham

transaction.atomic() doesn't seem to suffer from this issue. Is there any value in adding the test (an updated one is attached)?

Changed 11 days ago by Tim Graham

Attachment: 16682-atomic.diff added

comment:9 Changed 10 days ago by Tim Graham

Has patch: set
Resolution: fixed
Status: newclosed
Triage Stage: AcceptedReady for checkin

I created a PR and Aymeric indicated that adding the test is useful.

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