Close Transactions Properly (#45)

## What is the goal of this PR?
Properly close transactions that was leading to failing client-python tests, and ensure that tests that are failing clean up properly rather than exiting without closing transactions. 

## What are the changes implemented in this PR?
* Exhaust the gRPC iterator when the transaction is closed to ensure that `onCompleted()` is called on the server. Previously, we were emptying the input iterator and adding `None` to its queue, but this was not being consumed. Thus, the transactions was not being cleaned up on the server as the `transaction(stream)` RPC call was never completed cleanly.
* Utilise `addCleanup` to close transactions even if the tests fail - this is always executed regardless of test status.

Closes #44

Author: flyingsilverfin

grakn/service/Session/TransactionService.py

@@ -155,8 +155,8 @@ def send(self, request):
             self._add_request(request)
             response = next(self._response_iterator)
         except Exception as e: # specialize into different gRPC exceptions?
-            # on any GRPC exception, close the stream
-            self.close()
+            # invalidate this communicator, functionally this occurs automatically on exception (iterator not usable anymore)
+            self._closed = True
             raise GraknError("Server/network error: {0}\n\n generated from request: {1}".format(e, request))
 
         if response is None:
@@ -165,7 +165,13 @@ def send(self, request):
         return response
 
     def close(self):
-        with self._queue.mutex: # probably don't even need the mutex
-            self._queue.queue.clear()
-        self._queue.put(None)
-        self._closed = True
+        if not self._closed:
+            with self._queue.mutex: # probably don't even need the mutex
+                self._queue.queue.clear()
+            self._queue.put(None)
+            self._closed = True
+            # force exhaust the iterator so `onCompleted()` is called on the server
+            try:
+                next(self._response_iterator)
+            except StopIteration:
+                pass

tests/integration/test_concept.py

@@ -77,10 +77,11 @@ def tearDownClass(cls):
     def setUp(self):
         global session
         self.tx = session.transaction().write()
+        # functions called by `addCleanup` are reliably called independent of test pass or failure
+        self.addCleanup(self.cleanupTransaction, self.tx)
 
-    def tearDown(self):
-        self.tx.close()
-
+    def cleanupTransaction(self, tx):
+        tx.close()
 
 
 class test_Concept(test_concept_Base):
@@ -111,10 +112,12 @@ def test_re_delete_instance(self):
         none_car = self.tx.get_concept(car.id)
         self.assertIsNone(none_car)
 
-        with self.assertRaises(GraknError):
+        with self.assertRaises(GraknError) as context:
             car.delete()
 
-    
+        self.assertTrue("FAILED_PRECONDITION" in str(context.exception))
+
+
     def test_is_each_schema_type(self):
         car_type = self.tx.put_entity_type("car")
         car = car_type.create()

tests/integration/test_grakn.py

@@ -161,10 +161,13 @@ def tearDownClass(cls):
         client.close()
 
     def setUp(self):
+        global session
         self.tx = session.transaction().write()
+        # functions called by `addCleanup` are reliably called independent of test pass or failure
+        self.addCleanup(self.cleanupTransaction, self.tx)
 
-    def tearDown(self):
-        self.tx.close()
+    def cleanupTransaction(self, tx):
+        tx.close()