Flush cache after the current transaction is committed (#296) (#300)

waffle/models.py

@@ -6,7 +6,7 @@
 
 from django.conf import settings
 from django.contrib.auth.models import Group
-from django.db import models, router
+from django.db import models, router, transaction
 from django.utils import timezone
 from django.utils.encoding import python_2_unicode_compatible
 from django.utils.translation import ugettext_lazy as _
@@ -98,12 +98,18 @@ def flush(self):
     def save(self, *args, **kwargs):
         self.modified = timezone.now()
         ret = super(BaseModel, self).save(*args, **kwargs)
-        self.flush()
+        if hasattr(transaction, 'on_commit'):
+            transaction.on_commit(self.flush)
+        else:
+            self.flush()
         return ret
 
     def delete(self, *args, **kwargs):
         ret = super(BaseModel, self).delete(*args, **kwargs)
-        self.flush()
+        if hasattr(transaction, 'on_commit'):
+            transaction.on_commit(self.flush)
+        else:
+            self.flush()
         return ret
 
 

waffle/tests/test_testutils.py

@@ -3,14 +3,14 @@
 from decimal import Decimal
 
 from django.contrib.auth.models import AnonymousUser
-from django.test import TestCase, RequestFactory
+from django.test import TransactionTestCase, RequestFactory
 
 import waffle
 from waffle.models import Switch, Flag, Sample
 from waffle.testutils import override_switch, override_flag, override_sample
 
 
-class OverrideSwitchTests(TestCase):
+class OverrideSwitchTests(TransactionTestCase):
     def test_switch_existed_and_was_active(self):
         Switch.objects.create(name='foo', active=True)
 
@@ -94,7 +94,7 @@ def req():
     return r
 
 
-class OverrideFlagTests(TestCase):
+class OverrideFlagTests(TransactionTestCase):
     def test_flag_existed_and_was_active(self):
         Flag.objects.create(name='foo', everyone=True)
 
@@ -140,7 +140,7 @@ def test_flag_did_not_exist(self):
         assert not Flag.objects.filter(name='foo').exists()
 
 
-class OverrideSampleTests(TestCase):
+class OverrideSampleTests(TransactionTestCase):
     def test_sample_existed_and_was_100(self):
         Sample.objects.create(name='foo', percent='100.0')
 
@@ -190,7 +190,7 @@ def test_sample_did_not_exist(self):
 
 
 @override_switch('foo', active=False)
-class OverrideSwitchOnClassTests(TestCase):
+class OverrideSwitchOnClassTests(TransactionTestCase):
     def setUp(self):
         assert not Switch.objects.filter(name='foo').exists()
         Switch.objects.create(name='foo', active=True)
@@ -200,7 +200,7 @@ def test_undecorated_method_is_set_properly_for_switch(self):
 
 
 @override_flag('foo', active=False)
-class OverrideFlagOnClassTests(TestCase):
+class OverrideFlagOnClassTests(TransactionTestCase):
     def setUp(self):
         assert not Flag.objects.filter(name='foo').exists()
         Flag.objects.create(name='foo', everyone=True)
@@ -210,7 +210,7 @@ def test_undecorated_method_is_set_properly_for_flag(self):
 
 
 @override_sample('foo', active=False)
-class OverrideSampleOnClassTests(TestCase):
+class OverrideSampleOnClassTests(TransactionTestCase):
     def setUp(self):
         assert not Sample.objects.filter(name='foo').exists()
         Sample.objects.create(name='foo', percent='100.0')

waffle/tests/test_waffle.py

@@ -1,10 +1,13 @@
 from __future__ import unicode_literals
 
 import random
+import threading
+import unittest
 
+from django.conf import settings
 from django.contrib.auth.models import AnonymousUser, Group, User
-from django.db import connection
-from django.test import RequestFactory
+from django.db import connection, transaction
+from django.test import RequestFactory, TransactionTestCase
 from django.test.utils import override_settings
 
 import mock
@@ -394,3 +397,102 @@ def test_read_from_write_db(self):
             # The next read should now be directed to the write DB, ensuring
             # the cache and DB are in sync.
             assert waffle.sample_is_active(sample.name)
+
+
+class TransactionTestMixin(object):
+    """Mixin providing an abstract test case for writing in a transaction.
+    """
+    def create_toggle(self):
+        """Create an inactive feature toggle (i.e. flag, switch, sample)."""
+        raise NotImplementedError
+
+    def flip_toggle(self, toggle):
+        """Flip the toggle to an active state."""
+        raise NotImplementedError
+
+    def toggle_is_active(self, toggle):
+        """Use the built-in *_is_active helper to check the toggle's state."""
+        raise NotImplementedError
+
+    @unittest.skipIf('sqlite3' in settings.DATABASES['default']['ENGINE'],
+                     'This test uses threads, which the sqlite3 DB engine '
+                     'does not support.')
+    def test_flip_toggle_in_transaction(self):
+        """Wait to invalidate the cache until after the current transaction.
+
+        This test covers a potential race condition where, if the cache were
+        flushed in the middle of a transaction, the next read from the database
+        (before the transaction is committed) would get a stale value and cache
+        it. See #296 for more context.
+        """
+        toggle = self.create_toggle()
+        self.addCleanup(toggle.delete)
+
+        written_in_background_thread = threading.Event()
+        read_in_main_thread = threading.Event()
+
+        @transaction.atomic
+        def update_toggle():
+            self.flip_toggle(toggle)
+
+            # Signal to the main thread that the toggle has been updated, but
+            # the transaction is not yet committed.
+            written_in_background_thread.set()
+
+            # Pause here to allow the main thread to make an assertion.
+            read_in_main_thread.wait(timeout=1)
+
+        # Start a background thread to update the toggle in a transaction.
+        t = threading.Thread(target=update_toggle)
+        t.daemon = True
+        t.start()
+
+        # After the toggle is updated but before the transaction is committed,
+        # the cache will still have the previous value.
+        written_in_background_thread.wait(timeout=1)
+        assert not self.toggle_is_active(toggle)
+
+        # After the transaction is committed, the cache should have been
+        # invalidated, hence the next call to *_is_active should have the
+        # correct value.
+        read_in_main_thread.set()
+        t.join(timeout=1)
+        assert self.toggle_is_active(toggle)
+
+
+class FlagTransactionTests(TransactionTestMixin, TransactionTestCase):
+    def create_toggle(self):
+        return Flag.objects.create(name='transaction-flag-name',
+                                   everyone=False)
+
+    def flip_toggle(self, flag):
+        flag.everyone = True
+        flag.save()
+
+    def toggle_is_active(self, flag):
+        return waffle.flag_is_active(get(), flag.name)
+
+
+class SwitchTransactionTests(TransactionTestMixin, TransactionTestCase):
+    def create_toggle(self):
+        return Switch.objects.create(name='transaction-switch-name',
+                                     active=False)
+
+    def flip_toggle(self, switch):
+        switch.active = True
+        switch.save()
+
+    def toggle_is_active(self, switch):
+        return waffle.switch_is_active(switch.name)
+
+
+class SampleTransactionTests(TransactionTestMixin, TransactionTestCase):
+    def create_toggle(self):
+        return Sample.objects.create(name='transaction-sample-name', percent=0)
+
+    def flip_toggle(self, sample):
+        sample.percent = 100
+        sample.save()
+
+    def toggle_is_active(self, sample):
+        return waffle.sample_is_active(sample.name)