Repository: learningequality/kolibri
Issue: occasional zombie processes related to unclean shutdown


### Observed behavior

observed by @jamalex and @indirectlylit during testing of zerofconf work

> occasional zombie process happening when I CTRL-C a `kolibri start --foreground` server

This caused me some very confusing issues when I had closed all my terminals and Kolibri was still running in the background

### Expected behavior

Clean and complete shutdown

### User-facing consequences

Could get very difficult-to-debug behaviors if zombie processes are accessing DBs


### Steps to reproduce

unknown

### Context

0.13.0 beta 4

Changed files:
kolibri/core/tasks/job.py, kolibri/core/discovery/test/test_network_search.py, kolibri/core/discovery/utils/network/search.py, kolibri/core/public/utils.py, kolibri/core/test/test_key_urls.py, kolibri/core/public/test/test_api.py, kolibri/utils/server.py

diff --git a/kolibri/core/discovery/test/test_network_search.py b/kolibri/core/discovery/test/test_network_search.py
index 92b46322e9..4dcd2303b2 100644
--- a/kolibri/core/discovery/test/test_network_search.py
+++ b/kolibri/core/discovery/test/test_network_search.py
@@ -1,7 +1,7 @@
 import socket
 
 import mock
-from django.test import TestCase
+from django.test import TransactionTestCase
 from zeroconf import BadTypeInNameException
 from zeroconf import service_type_name
 from zeroconf import ServiceInfo
@@ -97,6 +97,10 @@ class MockZeroconf(Zeroconf):
             service_type_name(info.name)
 
 
+@mock.patch(
+    "kolibri.core.discovery.utils.network.search.get_device_info",
+    return_value={"instance_id": MOCK_ID},
+)
 @mock.patch(
     "kolibri.core.discovery.utils.network.search._is_port_open", lambda *a, **kw: True
 )
@@ -106,16 +110,16 @@ class MockZeroconf(Zeroconf):
     lambda: [MOCK_INTERFACE_IP],
 )
 @mock.patch("django.db.models.Manager.update_or_create")
-class TestNetworkSearch(TestCase):
+class TestNetworkSearch(TransactionTestCase):
     def test_initialize_zeroconf_listener(self, *mocks):
         assert ZEROCONF_STATE["listener"] is None
         initialize_zeroconf_listener()
         assert ZEROCONF_STATE["listener"] is not None
 
-    def test_register_zeroconf_service(self, mock_db):
+    def test_register_zeroconf_service(self, mock_db, get_device_mock):
         assert len(get_peer_instances()) == 0
         initialize_zeroconf_listener()
-        register_zeroconf_service(MOCK_PORT, {"instance_id": MOCK_ID})
+        register_zeroconf_service(MOCK_PORT)
         assert [x for x in get_peer_instances()] == [
             {
                 "id": MOCK_ID,
@@ -135,7 +139,7 @@ class TestNetworkSearch(TestCase):
                 ),
             }
         ]
-        register_zeroconf_service(MOCK_PORT, {"instance_id": MOCK_ID})
+        register_zeroconf_service(MOCK_PORT)
         unregister_zeroconf_service()
         assert len(get_peer_instances()) == 0
 
diff --git a/kolibri/core/discovery/utils/network/search.py b/kolibri/core/discovery/utils/network/search.py
index 99d480a357..f9b038735a 100644
--- a/kolibri/core/discovery/utils/network/search.py
+++ b/kolibri/core/discovery/utils/network/search.py
@@ -5,6 +5,7 @@ import socket
 from contextlib import closing
 
 from django.core.exceptions import ValidationError
+from django.db import connection
 from zeroconf import get_all_addresses
 from zeroconf import NonUniqueNameException
 from zeroconf import ServiceInfo
@@ -12,6 +13,7 @@ from zeroconf import USE_IP_OF_OUTGOING_INTERFACE
 from zeroconf import Zeroconf
 
 from kolibri.core.discovery.models import DynamicNetworkLocation
+from kolibri.core.public.utils import get_device_info
 
 logger = logging.getLogger(__name__)
 
@@ -153,6 +155,8 @@ class KolibriZeroconfListener(object):
                     )
                     % (id, self.instances[id], traceback.format_exc(limit=1)),
                 )
+            finally:
+                connection.close()
 
     def remove_service(self, zeroconf, type, name):
         id = _id_from_name(name)
@@ -165,14 +169,13 @@ class KolibriZeroconfListener(object):
             pass
 
         DynamicNetworkLocation.objects.filter(pk=id).delete()
+        connection.close()
 
 
-def cleanup_database():
+def register_zeroconf_service(port):
+    device_info = get_device_info()
     DynamicNetworkLocation.objects.all().delete()
-
-
-def register_zeroconf_service(port, device_info):
-    cleanup_database()
+    connection.close()
 
     id = device_info.get("instance_id")
 
diff --git a/kolibri/core/public/test/test_api.py b/kolibri/core/public/test/test_api.py
index 8ee48cb10a..9705d43827 100644
--- a/kolibri/core/public/test/test_api.py
+++ b/kolibri/core/public/test/test_api.py
@@ -5,7 +5,7 @@ import factory
 from django.core.urlresolvers import reverse
 from le_utils.constants import content_kinds
 from morango.models import InstanceIDModel
-from rest_framework.test import APITestCase
+from rest_framework.test import APITransactionTestCase
 from six import iteritems
 
 import kolibri
@@ -78,7 +78,7 @@ def create_mini_channel(
     )
 
 
-class PublicAPITestCase(APITestCase):
+class PublicAPITestCase(APITransactionTestCase):
     """
     IMPORTANT: These tests are to never be changed. They are enforcing a
     public API contract. If the tests fail, then the implementation needs
diff --git a/kolibri/core/public/utils.py b/kolibri/core/public/utils.py
index 5f68eac7f9..c3c95cc8f4 100644
--- a/kolibri/core/public/utils.py
+++ b/kolibri/core/public/utils.py
@@ -17,5 +17,4 @@ def get_device_info():
         "device_name": instance_model.hostname,
         "operating_system": platform.system(),
     }
-
     return info
diff --git a/kolibri/core/tasks/job.py b/kolibri/core/tasks/job.py
index bb5685e605..ec243d7e00 100644
--- a/kolibri/core/tasks/job.py
+++ b/kolibri/core/tasks/job.py
@@ -2,6 +2,8 @@ import copy
 import logging
 import uuid
 
+from django.db import connection
+
 from kolibri.core.tasks.utils import current_state_tracker
 from kolibri.core.tasks.utils import import_stringified_func
 from kolibri.core.tasks.utils import stringify_func
@@ -181,6 +183,9 @@ class Job(object):
 
             setattr(current_state_tracker, "job", None)
 
+            # Close any django connections opened here
+            connection.close()
+
             return result
 
         return y
diff --git a/kolibri/core/test/test_key_urls.py b/kolibri/core/test/test_key_urls.py
index 1ea4651d37..019af56087 100644
--- a/kolibri/core/test/test_key_urls.py
+++ b/kolibri/core/test/test_key_urls.py
@@ -6,6 +6,7 @@ from django.core.urlresolvers import reverse
 from django.urls.exceptions import NoReverseMatch
 from mock import patch
 from rest_framework.test import APITestCase
+from rest_framework.test import APITransactionTestCase
 
 from kolibri.core.auth.constants import role_kinds
 from kolibri.core.auth.test.helpers import create_superuser
@@ -47,7 +48,7 @@ class KolibriTagNavigationTestCase(APITestCase):
         )
 
 
-class AllUrlsTest(APITestCase):
+class AllUrlsTest(APITransactionTestCase):
 
     # Allow codes that may indicate a poorly formed response
     # 412 is returned from endpoints that have required GET params when these are not supplied
diff --git a/kolibri/utils/server.py b/kolibri/utils/server.py
index 7c1010c235..e9f5b8230a 100644
--- a/kolibri/utils/server.py
+++ b/kolibri/utils/server.py
@@ -99,15 +99,11 @@ class ServicesPlugin(SimplePlugin):
         scheduler.start_scheduler()
 
         # Register the Kolibri zeroconf service so it will be discoverable on the network
-        from kolibri.core.public.utils import get_device_info
-
-        device_info = get_device_info()
-
         from kolibri.core.discovery.utils.network.search import (
             register_zeroconf_service,
         )
 
-        register_zeroconf_service(port=self.port, device_info=device_info)
+        register_zeroconf_service(port=self.port)
 
     def stop(self):
         if self.workers is not None: