From f02d508370be3f48f0d61541089a4aebe4d88c11 Mon Sep 17 00:00:00 2001
From: Nick Alexander <nalexander@mozilla.com>
Date: Thu, 13 Oct 2016 12:45:29 -0700
Subject: [PATCH] Review comment: ensure <transact! after <close is rejected.

---
 src/common/datomish/transact.cljc | 27 ++++++++++++++++++---------
 test/datomish/transact_test.cljc  | 21 +++++++++++++++++++++
 2 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/src/common/datomish/transact.cljc b/src/common/datomish/transact.cljc
index 68da51dc..04c56e69 100644
--- a/src/common/datomish/transact.cljc
+++ b/src/common/datomish/transact.cljc
@@ -35,7 +35,9 @@
 (defprotocol IConnection
   (close
     [conn]
-    "Close this connection. Returns a pair channel of [nil error].")
+    "Close this connection. Returns a pair channel of [nil error].
+
+    Closing a closed connection is a no-op.")
 
   (db
     [conn]
@@ -45,11 +47,15 @@
     [conn]
     "Get the full transaction history DB associated with this connection."))
 
-(defrecord Connection [current-db transact-chan]
+(defrecord Connection [closed? current-db transact-chan]
   IConnection
   (close [conn]
-    (a/close! (:transact-chan conn))
-    (db/close-db @(:current-db conn)))
+    (if (compare-and-set! (:closed? conn) false true)
+      (do
+        ;; This immediately stops <transact! enqueueing new work.
+        (a/close! (:transact-chan conn))
+        (db/close-db @(:current-db conn)))
+      (go [nil nil])))
 
   (db [conn] @(:current-db conn))
 
@@ -112,7 +118,8 @@
         (a/mult listener-source) ;; Just for tapping.
 
         connection
-        (map->Connection {:current-db       (atom db)
+        (map->Connection {:closed?          (atom false)
+                          :current-db       (atom db)
                           :listener-source  listener-source
                           :listener-mult    listener-mult
                           :transact-chan    (a/chan (util/unlimited-buffer))
@@ -576,10 +583,12 @@
    {:pre [(conn? conn)]}
    ;; Any race to put! is a real race between callers of <transact!.  We can't just park on put!,
    ;; because the parked putter that is woken is non-deterministic.
-   (a/put! (:transact-chan conn) [tx-data result close?])
-   (go-pair
-     ;; We want to return a pair-chan, no matter what kind of channel result is.
-     (<? result))))
+   (let [closed? (not (a/put! (:transact-chan conn) [tx-data result close?]))]
+     (go-pair
+       ;; We want to return a pair-chan, no matter what kind of channel result is.
+       (if closed?
+         (raise "Connection is closed" {:error :transact/connection-closed})
+         (<? result))))))
 
 (defn- start-transactor [conn]
   (let [token-chan (a/chan 1)]
diff --git a/test/datomish/transact_test.cljc b/test/datomish/transact_test.cljc
index 34970f8b..ed211711 100644
--- a/test/datomish/transact_test.cljc
+++ b/test/datomish/transact_test.cljc
@@ -221,4 +221,25 @@
       (is (= (<? (<datoms-after (d/db conn) tx0))
              #{[101 :name "Petr"]})))))
 
+;; We don't use deftest-db in order to be able to close the connection ourselves.
+(deftest-async test-transact-after-close
+  (with-tempfile [t (tempfile)]
+    (let [conn (<? (d/<connect t))
+          {tx0 :tx} (<? (d/<transact! conn test-schema))]
+      (try
+        (testing "transaction before close is applied"
+          (<? (d/<transact! conn [{:db/id 101 :name "Petr"}]))
+          (is (= (<? (<datoms-after (d/db conn) tx0))
+                 #{[101 :name "Petr"]})))
+        (finally
+          (<? (d/<close conn))))
+
+      (testing "transact after close throws"
+        (is (thrown-with-msg?
+              ExceptionInfo #"Connection is closed"
+              (<? (d/<transact! conn [{:db/id (d/id-literal :db.part/user -1) :name "Petr"}])))))
+
+      ;; Closing a closed connection is a no-op.
+      (<? (d/<close conn)))))
+
 #_ (time (t/run-tests))