Merge pull request #183 from metosin/coercion_memoization_fix

Coercion memoization fix

Author: ikitommi

CHANGELOG.md

@@ -1,3 +1,14 @@
+## 0.24.3 (13.12.2015)
+
+* coercer-cache is now per Route instead beeing global and based on a FIFO size 100 cache. Avoids potential memory
+leaks when using anonymous coercion matchers (which never hit the cache).
+
+* Updated deps:
+
+```clj
+[prismatic/schema "1.0.4"] is available but we use "1.0.3"
+```
+
 ## 0.24.2 (8.12.2015)
 
 **[compare](https://github.com/metosin/compojure-api/compare/0.24.1...0.24.2)**

project.clj

@@ -9,7 +9,7 @@
                  [potemkin "0.4.2"]
                  [cheshire "5.5.0"]
                  [compojure "1.4.0"]
-                 [prismatic/schema "1.0.3"]
+                 [prismatic/schema "1.0.4"]
                  [org.tobereplaced/lettercase "1.0.0"]
                  [frankiesardo/linked "1.2.6"]
                  [metosin/ring-http-response "0.6.5"]

src/compojure/api/meta.clj

@@ -13,7 +13,9 @@
             [schema.core :as s]
             [schema.coerce :as sc]
             [schema.utils :as su]
-            [schema-tools.core :as st]))
+            [schema-tools.core :as st]
+            [linked.core :as linked]
+            [compojure.api.impl.logging :as logging]))
 
 ;;
 ;; Meta Evil
@@ -27,6 +29,10 @@
   "lexically bound meta-data for handlers."
   '+compojure-api-meta+)
 
+(def +compojure-api-coercer+
+  "lexically bound (caching) coercer for handlers."
+  '+compojure-api-coercer+)
+
 (defmacro meta-container [meta & form]
   `(let [accumulated-meta# (get-local +compojure-api-meta+)
          ~'+compojure-api-meta+ (deep-merge accumulated-meta# ~meta)]
@@ -47,7 +53,24 @@
 ;; Schema
 ;;
 
-(def memoized-coercer (memoize sc/coercer))
+(defn memoized-coercer
+  "Returns a memoized version of a referentially transparent coercer fn. The
+  memoized version of the function keeps a cache of the mapping from arguments
+  to results and, when calls with the same arguments are repeated often, has
+  higher performance at the expense of higher memory use. FIFO with 100 entries.
+  Cache will be filled if anonymous coercers are used (does not match the cache)"
+  []
+  (let [cache (atom (linked/map))
+        cache-size 100]
+    (fn [& args]
+      (or (@cache args)
+          (let [coercer (apply sc/coercer args)]
+            (swap! cache (fn [mem]
+                           (let [mem (assoc mem args coercer)]
+                             (if (>= (count mem) cache-size)
+                               (dissoc mem (-> mem first first))
+                               mem))))
+            coercer)))))
 
 (defn strict [schema]
   (dissoc schema 'schema.core/Keyword))
@@ -57,12 +80,12 @@
     (fnk-impl/letk-input-schema-and-body-form
       nil (with-meta bind {:schema s/Any}) [] nil)))
 
-(defn body-coercer-middleware [handler responses]
+(defn body-coercer-middleware [handler coercer responses]
   (fn [request]
     (if-let [{:keys [status] :as response} (handler request)]
       (if-let [schema (:schema (responses status))]
         (if-let [matcher (:response (mw/get-coercion-matcher-provider request))]
-          (let [coerce (memoized-coercer (value-of schema) matcher)
+          (let [coerce (coercer (value-of schema) matcher)
                 body (coerce (:body response))]
             (if (su/error? body)
               (throw+ (assoc body :type ::ex/response-validation))
@@ -79,7 +102,7 @@
   (assert (not (#{:query :json} type)) (str type " is DEPRECATED since 0.22.0. Use :body or :string instead."))
   `(let [value# (keywordize-keys (~key ~+compojure-api-request+))]
      (if-let [matcher# (~type (mw/get-coercion-matcher-provider ~+compojure-api-request+))]
-       (let [coerce# (memoized-coercer ~schema matcher#)
+       (let [coerce# (~+compojure-api-coercer+ ~schema matcher#)
              result# (coerce# value#)]
          (if (su/error? result#)
            (throw+ (assoc result# :type ::ex/request-validation))
@@ -116,7 +139,7 @@
 
 (defmulti restructure-param
   "Restructures a key value pair in smart routes. By default the key
-   is consumed form the :parameters map in acc. k = given key, v = value."
+  is consumed form the :parameters map in acc. k = given key, v = value."
   (fn [k v acc] k))
 
 ;;
@@ -337,11 +360,14 @@
           (map-of lets letks responses middlewares parameters body)
           parameters)
 
+        pre-lets [+compojure-api-coercer+ `(memoized-coercer)]
+
         body `(~body-wrap ~@body)
         body (if (seq letks) `(letk ~letks ~body) body)
         body (if (seq lets) `(let ~lets ~body) body)
         body (if (seq middlewares) `(route-middlewares ~middlewares ~body ~arg) body)
         body (if (seq parameters) `(meta-container ~parameters ~body) body)
         body `(~method-symbol ~path ~arg-with-request ~body)
-        body (if responses `(body-coercer-middleware ~body ~responses) body)]
+        body (if responses `(body-coercer-middleware ~body ~+compojure-api-coercer+ ~responses) body)
+        body (if (seq pre-lets) `(let ~pre-lets ~body) body)]
     body))

test/compojure/api/coercion_test.clj

@@ -43,7 +43,7 @@
             body => {:beers ["ipa" "apa"]})))
 
       (fact "body-coersion can ba disabled"
-        (let [no-body-coercion (fn [_] (dissoc mw/default-coercion-matchers :body))
+        (let [no-body-coercion (constantly (dissoc mw/default-coercion-matchers :body))
               app (api
                     {:coercion no-body-coercion}
                     beer-route)]
@@ -52,7 +52,7 @@
             body => {:beers ["ipa" "apa" "ipa"]})))
 
       (fact "body-coersion can ba changed"
-        (let [nop-body-coercion (fn [_] (assoc mw/default-coercion-matchers :body (constantly nil)))
+        (let [nop-body-coercion (constantly (assoc mw/default-coercion-matchers :body (constantly nil)))
               app (api
                     {:coercion nop-body-coercion}
                     beer-route)]
@@ -71,7 +71,7 @@
             body => {:i 10})))
 
       (fact "query-coersion can ba disabled"
-        (let [no-query-coercion (fn [_] (dissoc mw/default-coercion-matchers :string))
+        (let [no-query-coercion (constantly (dissoc mw/default-coercion-matchers :string))
               app (api
                     {:coercion no-query-coercion}
                     query-route)]
@@ -80,7 +80,7 @@
             body => {:i "10"})))
 
       (fact "query-coersion can ba changed"
-        (let [nop-query-coercion (fn [_] (assoc mw/default-coercion-matchers :string (constantly nil)))
+        (let [nop-query-coercion (constantly (assoc mw/default-coercion-matchers :string (constantly nil)))
               app (api
                     {:coercion nop-query-coercion}
                     query-route)]
@@ -92,7 +92,7 @@
                   :query-params [i :- s/Int]
                   (ok {:i i}))
                 (GET* "/disabled-coercion" []
-                  :coercion (fn [_] (assoc mw/default-coercion-matchers :string (constantly nil)))
+                  :coercion (constantly (assoc mw/default-coercion-matchers :string (constantly nil)))
                   :query-params [i :- s/Int]
                   (ok {:i i}))
                 (GET* "/no-coercion" []
@@ -111,4 +111,34 @@
       (fact "no coercion"
         (let [[status body] (get* app "/no-coercion" {:i 10})]
           status => 200
-          body => {:i "10"})))))
+          body => {:i "10"}))))
+
+  (fact "anonymous matchers, with 100+ calls to same endpoint"
+
+    #_(fact "at api-level, matcher is reused and the coercion matcher cache is not filled"
+      (let [app (api
+                  {:coercion (constantly mw/default-coercion-matchers)}
+                  (GET* "/anonymous" []
+                    :query-params [i :- s/Str]
+                    (ok {:i i})))]
+
+        (dotimes [_ 200]
+          (let [[status body] (get* app "/anonymous" {:i "10"})]
+            status => 200
+            body => {:i "10"})) => nil
+        (provided
+          (compojure.api.impl.logging/log! & anything) => irrelevant :times 0)))
+
+    #_(fact "at route-level, matcher is NOT reused and the the coercion matcher cache is filled"
+      (let [app (api
+                  (GET* "/anonymous" []
+                    :coercion (constantly (assoc mw/default-coercion-matchers :string (constantly nil)))
+                    :query-params [i :- s/Str]
+                    (ok {:i i})))]
+
+        (dotimes [_ 200]
+          (let [[status body] (get* app "/anonymous" {:i "10"})]
+            status => 200
+            body => {:i "10"})) => nil
+        (provided
+          (compojure.api.impl.logging/log! & anything) => irrelevant :times 1)))))