Opened 13 years ago

Closed 8 years ago

Last modified 8 years 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: 1.11
Cc: Mariusz Felisiak Triage Stage: Accepted
Has patch: no 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 13 years ago.
Failing test
16682-atomic.diff (1.0 KB ) - added by Tim Graham 8 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 by Aymeric Augustin, 13 years ago

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 by Claude Paroz, 13 years ago

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 by Claude Paroz, 13 years ago

#17624 is a duplicate with a patch

by Claude Paroz, 13 years ago

Attachment: 16682-test.diff added

Failing test

comment:6 by Claude Paroz, 13 years ago

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 by Aymeric Augustin, 12 years ago

Status: reopenednew

comment:8 by Tim Graham, 8 years ago

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

by Tim Graham, 8 years ago

Attachment: 16682-atomic.diff added

comment:9 by Tim Graham, 8 years ago

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.

comment:10 by Tim Graham, 8 years ago

Has patch: unset
Keywords: 1.11 added
Resolution: fixed
Status: closednew
Triage Stage: Ready for checkinAccepted

Reopening as the test hangs on the last line when I run it on Oracle (using the Oracle DB Developer VM). I haven't seen any problems with it on Jenkins. Can anyone else reproduce that and/or have an idea about the cause?

comment:11 by Tim Graham, 8 years ago

Cc: Mariusz Felisiak added

Mariusz, are you able to reproduce the hang on Oracle? Any ideas?

comment:12 by Mariusz Felisiak, 8 years ago

I wasn't able to reproduce the hang on Oracle 11g/12c. I will try later with Oracle DB Developer VM. Small suggestion is to use signal.SIGINT instead of signal number.

--- a/tests/transactions/tests.py
+++ b/tests/transactions/tests.py
@@ -1,6 +1,7 @@
 from __future__ import unicode_literals
 
 import os
+import signal
 import sys
 import threading
 import time
@@ -225,8 +226,8 @@ class AtomicTests(TransactionTestCase):
             with transaction.atomic():
                 Reporter.objects.create(first_name='Tintin')
                 # Send SIGINT (simulate Ctrl-C). One call isn't enough.
-                os.kill(os.getpid(), 2)
-                os.kill(os.getpid(), 2)
+                os.kill(os.getpid(), signal.SIGINT)
+                os.kill(os.getpid(), signal.SIGINT)
         except KeyboardInterrupt:
             pass
         self.assertEqual(Reporter.objects.all().count(), 0)

comment:13 by Mariusz Felisiak, 8 years ago

I confirmed that test_rollback_on_keyboardinterrupt hangs on Oracle from Oracle DB Developer VM. I will try to figure it out in the next days.

comment:14 by Tim Graham <timograham@…>, 8 years ago

In d391b3a:

Refs #16682 -- Replaced signal number with equivalent signal.SIGINT.

comment:15 by Mariusz Felisiak, 8 years ago

IMO it is not a Django issue. Test hangs on ​rollback, hence it is probably Oracle or cx_Oracle bug.

comment:16 by Tim Graham, 8 years ago

Thanks for investigating. Since this is the only documented way we have of running the tests on Oracle for development, is there a chance we could skip the test when running with the Developer Days VM? Maybe the Oracle version is different from the version on Jenkins, for example.

comment:17 by Mariusz Felisiak, 8 years ago

Oracle DB is in the same version in both cases (12.1.0.2.0). I also checked DB params and there is nothing unusual in them. I don't know where is the source of the problem.

comment:18 by Josh Smeaton, 8 years ago

I just hit this too.

Dev Days VM: 2016-06-02_09_53_26 64 bit.
Tests OS: OSX 10.12.2
Python 3.5.2
cx_Oracle: 5.2.1

comment:19 by Tim Graham, 8 years ago

Resolution: fixed
Status: newclosed

I guess there's nothing to be done. Hopefully it's solved in a future version of the Oracle VM.

comment:20 by Tim Graham, 8 years ago

There's another problem with this test. When running ./tests/runtests.py transactions --parallel=1, this test halts execution so that only 26 tests are run. After commenting this test out, the same command runs 72 tests. Maybe it's better to remove this test for now.

comment:21 by Tim Graham <timograham@…>, 8 years ago

In dfbdba92:

Reverted "Refs #16682 -- Tested transaction.atomic() with KeyboardInterrupt."

This reverts commit d895fc9ac01db3d3420aa7c943949fe17b3ce028 since the
test is problematic as described in the ticket.

comment:22 by Tim Graham <timograham@…>, 8 years ago

In e9ba856:

[1.11.x] Reverted "Refs #16682 -- Tested transaction.atomic() with KeyboardInterrupt."

This reverts commit d895fc9ac01db3d3420aa7c943949fe17b3ce028 since the
test is problematic as described in the ticket.

Backport of dfbdba924fd7cf12ce92f7a86b97590d25b75733 from master

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