replikativ / konserve

A clojuresque key-value/document store protocol with core.async.
Eclipse Public License 1.0
299 stars 25 forks source link

File descriptor leak in `keys` for filesystem store? #60

Closed bwallinspire closed 2 years ago

bwallinspire commented 2 years ago

OS: Linux version 5.11.4-1-rt11-MANJARO Konserve: 0.6.0-alpha3 Clojure: 1.10.0-alpha6

With the included filesystem store backend, each call to konserve.core/keys seems to create multiple new file descriptors. E.g.:

(ns konserve-fdbug-mwe.core
  (:require
   [clojure.core.async :as async :refer [<!!]]
   [konserve.filestore :refer [new-fs-store]]
   [konserve.core :as k]
   [clj-pid.core :as pid]))

(defn count-fd
  "Prints count of file-descriptors in /proc/<this process pid>/fd"
  []
  (let [fd-dir (str "/proc/" (pid/current) "/fd")]
    (count (file-seq (clojure.java.io/file fd-dir)))))

(def store (<!! (new-fs-store "/tmp/test-store")))

(println "Before calling k/keys #fd: " (count-fd))
(prn (map :key (<!! (k/keys store))))
(println "After calling k/keys #fd: " (count-fd))

Example output:

Before calling k/keys #fd: 103
...
After calling k/keys #fd: 107

Seems to happen whether or not the store is new/empty or not, and all the duplicate file descriptors point to "/tmp/test-store".

Is this a valid way to use keys?

whilo commented 2 years ago

Hey @bwallinspire. Yes, this is a correct use. You did not include your pid namespace. Could you check whether you can reproduce the problem on the pending feature branch https://github.com/replikativ/konserve/pull/57 with both synchronous and asynchronous protocol variants?

Meaning (k/keys store {:sync? true}) and (<!! (k/keys store {:sync? false})). The latter is still the default behaviour if you do not pass the additional :sync? option.

bwallinspire commented 2 years ago

Thanks for the quick response @whilo! clj-pid was a dependency used just to get the process id to check open file-descriptors: clj-pid 0.1.2. There is probably a better/platform-independent way -- I am pretty new to Java/Clojure-land hence my limited insights here.

On #57, it seems the problem still does reproduce for both asynchronous and synchronous variants.

whilo commented 2 years ago

Thanks for reporting! I fixed it in the PR directly. Is rt11 a realtime Linux kernel version?

bwallinspire commented 2 years ago

Great! Do you have an estimate for when #57 is merged and released?

Yes, realtime kernel -- I have been playing around with some live audio processing :)

whilo commented 2 years ago

It is merged now, you should be able to use it as 0.6.0-SNAPSHOT already, but our other backends are not yet ported. So for now you can use the filestore and memory store on the JVM.