[Fix #278] Memory leak in find-symbol

Author: mbuczko

project.clj

@@ -6,7 +6,7 @@
   :dependencies [[nrepl "0.5.3"]
                  ^:inline-dep [http-kit "2.3.0"]
                  ^:inline-dep [cheshire "5.8.1"]
-                 ^:inline-dep [org.clojure/tools.analyzer.jvm "0.7.2"]
+                 ^:inline-dep [org.clojure/tools.analyzer.jvm "0.7.3"]
                  ^:inline-dep [org.clojure/tools.namespace "0.3.1" :exclusions [org.clojure/tools.reader]]
                  ^:inline-dep [org.clojure/tools.reader "1.3.2"]
                  ^:inline-dep [cider/orchard "0.5.4"]

src/refactor_nrepl/analyzer.clj

@@ -28,16 +28,17 @@
   a map of the aliases for the namespace as the second element
   in the same format as ns-aliases"
   [body]
-  (let [ns-decl (read-ns-decl (PushbackReader. (java.io.StringReader. body)))
-        aliases (->> ns-decl
-                     (filter list?)
-                     (some #(when (#{:require} (first %)) %))
-                     rest
-                     (remove symbol?)
-                     (filter #(contains? (set %) :as))
-                     (#(zipmap (map (partial get-alias nil) %)
-                               (map first %))))]
-    [(second ns-decl) aliases]))
+  (with-open [string-reader (java.io.StringReader. body)]
+    (let [ns-decl (read-ns-decl (PushbackReader. string-reader))
+          aliases (->> ns-decl
+                       (filter list?)
+                       (some #(when (#{:require} (first %)) %))
+                       rest
+                       (remove symbol?)
+                       (filter #(contains? (set %) :as))
+                       (#(zipmap (map (partial get-alias nil) %)
+                                 (map first %))))]
+      [(second ns-decl) aliases])))
 
 (defn- noop-macroexpand-1 [form]
   form)

src/refactor_nrepl/find/find_symbol.clj

@@ -24,12 +24,12 @@
                       (str/replace "#'" "")
                       (str/replace "clojure.core/" "")))
         full-class (get alias-info class class)]
-    (str/join "/" (remove nil? [full-class (:field node)]))))
+    (str/join "/" (remove nil? [(if (map? full-class) "" full-class) (:field node)]))))
 
 (defn- contains-var?
-  "Checks if the var of `node` is present in the `var-set`."
-  [vars-set alias-info node]
-  (vars-set (node->var alias-info node)))
+  "Checks if the var of `node` is same as given `var-name`"
+  [var-name alias-info node]
+  (= var-name (node->var alias-info node)))
 
 (defn present-before-expansion?
   "returns true if node is not result of macro expansion or if it is and it contains
@@ -48,8 +48,7 @@
 
 (defn- find-nodes
   "Filters `ast` with `pred` and returns a list of vectors with line-beg, line-end,
-  colum-beg, column-end and the result of applying pred to the node for each
-  node in the AST.
+  colum-beg, column-end for each node in the AST.
 
   if name present macro call sites are checked if they contained name before macro expansion"
   ([asts pred]
@@ -58,8 +57,7 @@
         (map (juxt (comp :line :env)
                    (comp :end-line :env)
                    (comp :column :env)
-                   (comp :end-column :env)
-                   pred))
+                   (comp :end-column :env)))
         (map #(zipmap [:line-beg :line-end :col-beg :col-end] %))))
   ([name asts pred]
    (find-nodes (map #(postwalk % (partial dissoc-macro-nodes name)) asts) pred)))
@@ -81,7 +79,7 @@
          var-name)))
 
 (defn- contains-var-or-const? [var-name alias-info node]
-  (or (contains-var? #{var-name} alias-info node)
+  (or (contains-var? var-name alias-info node)
       (contains-const? var-name alias-info node)))
 
 (defn- find-symbol-in-ast [name asts]
@@ -103,24 +101,24 @@
          (str/join "\n")
          str/trim)))
 
-(defn- find-symbol-in-file [fully-qualified-name ignore-errors ^File file]
+(defn- find-symbol-in-file [fully-qualified-name ignore-errors referred-syms ^File file]
   (let [file-content (slurp file)
         locs (try (->> (ana/ns-ast file-content)
                        (find-symbol-in-ast fully-qualified-name)
                        (filter :line-beg))
                   (catch Exception e
                     (when-not ignore-errors
                       (throw e))))
-        locs (concat locs
-                     (some->
-                      (libspecs/referred-syms-by-file&fullname)
-                      (get-in [:clj (str file) fully-qualified-name])
-                      meta
-                      ((fn [{:keys [line column end-line end-column]}]
-                         (list {:line-beg line
-                                :line-end end-line
-                                :col-beg column
-                                :col-end end-column})))))
+        locs (into
+              locs (some->
+                    referred-syms
+                    (get-in [:clj (str file) fully-qualified-name])
+                    meta
+                    ((fn [{:keys [line column end-line end-column]}]
+                       (list {:line-beg line
+                              :line-end end-line
+                              :col-beg column
+                              :col-end end-column})))))
         gather (fn [info]
                  (merge info
                         {:file (.getCanonicalPath file)
@@ -134,10 +132,11 @@
   (let [namespace (or ns (core/ns-from-string (slurp file)))
         fully-qualified-name (if (= namespace "clojure.core")
                                var-name
-                               (str/join "/" [namespace var-name]))]
+                               (str/join "/" [namespace var-name]))
+        referred-syms (libspecs/referred-syms-by-file&fullname)]
     (->> (core/dirs-on-classpath)
          (mapcat (partial core/find-in-dir (some-fn core/clj-file? core/cljc-file?)))
-         (mapcat (partial find-symbol-in-file fully-qualified-name ignore-errors)))))
+         (mapcat (partial find-symbol-in-file fully-qualified-name ignore-errors referred-syms)))))
 
 (defn- get&read-enclosing-sexps
   [file-content {:keys [^long line-beg ^long col-beg]}]
@@ -165,7 +164,7 @@
       res)))
 
 (defn- occurrence-for-optmap-default
-  [var-name [{:keys [line-beg col-beg] :as orig-occurrence} [_ _ ^String level2-string _]]]
+  [var-name [orig-occurrence [_ _ ^String level2-string _]]]
   (let [var-positions (re-pos (re-pattern (format "\\W%s\\W" var-name)) level2-string)
         ^long var-default-pos (first (second var-positions))
         newline-cnt (reduce (fn [cnt char] (if (= char \newline) (inc (long cnt)) cnt)) 0 (.substring level2-string 0 var-default-pos))
@@ -228,10 +227,6 @@
                  (map (partial occurrence-for-optmap-default var-name)))]
         (sort-by :line-beg (concat local-occurrences optmap-def-occurrences))))))
 
-(defn- to-find-symbol-result
-  [{:keys [line-beg line-end col-beg col-end name file match]}]
-  [line-beg line-end col-beg col-end name file match])
-
 (defn find-symbol [{:keys [file ns name line column ignore-errors]}]
   (core/throw-unless-clj-file file)
   (let [ignore-errors? (= ignore-errors "true")
@@ -240,9 +235,15 @@
                      distinct
                      (remove find-util/spurious?)
                      future)]
+
     (or
      ;; find-local-symbol is the fastest of the three
-     (not-empty (remove find-util/spurious? (distinct (find-local-symbol file name line column))))
+     ;; if result is not empty, there is no point in keeping `find-macro` and `find-global-symbol` futures still active
+     (when-let [result (not-empty (remove find-util/spurious? (distinct (find-local-symbol file name line column))))]
+       (future-cancel macros)
+       (future-cancel globals)
+       result)
+
      ;; find-macros has to be checked first because find-global-symbol
      ;; can return spurious hits for some macro definitions
      @macros

src/refactor_nrepl/middleware.clj

@@ -54,7 +54,7 @@
   `(with-errors-being-passed-on ~transport ~msg
      (config/with-config ~msg
        (transport/send ~transport
-                       (response-for ~msg ~(apply hash-map :status :done kvs))))))
+                       (response-for ~msg ~(apply hash-map kvs))))))
 
 (defn- bencode-friendly-data [data]
   ;; Bencode only supports byte strings, integers, lists and maps.
@@ -94,15 +94,10 @@
    (require-and-resolve 'refactor-nrepl.find.find-symbol/find-symbol)))
 
 (defn- find-symbol-reply [{:keys [transport] :as msg}]
-  (config/with-config msg
-    (with-errors-being-passed-on transport msg
-      (let [occurrences (@find-symbol msg)]
-        (doseq [occurrence occurrences
-                :let [response (serialize-response msg occurrence)]]
-          (transport/send transport
-                          (response-for msg :occurrence response)))
-        (transport/send transport (response-for msg :count (count occurrences)
-                                                :status :done))))))
+  (let [occurrences (@find-symbol msg)]
+    (doseq [occurrence occurrences]
+      (reply transport msg :occurrence (serialize-response msg occurrence)))
+    (reply transport msg :count (count occurrences) :status :done)))
 
 (def ^:private artifact-list
   (delay (require-and-resolve 'refactor-nrepl.artifacts/artifact-list)))
@@ -138,7 +133,7 @@
    (require-and-resolve 'refactor-nrepl.find.find-locals/find-used-locals)))
 
 (defn- find-used-locals-reply [{:keys [transport] :as msg}]
-  (reply transport msg :used-locals (@find-used-locals msg)))
+  (reply transport msg :used-locals (@find-used-locals msg) :status :done))
 
 (defn- version-reply [{:keys [transport] :as msg}]
   (reply transport msg :status :done :version (core/version)))