Merge pull request #3898 from clojure-emacs/bound-completed-requests

Bound nrepl-completed-requests with a FIFO cap

Author: bbatsov

CHANGELOG.md

@@ -24,6 +24,7 @@
 - Plug five request-id leaks in raw nREPL response handlers (`cider/test-stacktrace`, `cider/test-var-query`, `cider/analyze-last-stacktrace`, `cider/ns-reload`, `cider/get-state`). They never called `nrepl--mark-id-completed`, so their ids accumulated in the connection's `nrepl-pending-requests` for the lifetime of the session.
 - A response-handler error or an unrecognized response id no longer aborts the nREPL response queue. `nrepl-client-filter` wraps the dispatch call in `with-demoted-errors`, and the no-callback case in `nrepl--dispatch-response` is now a `message` instead of a hard error -- so a single misbehaving callback can no longer drop later responses on the floor.
 - `nrepl-client-sentinel` now tears down the SSH tunnel buffer/process when the client connection closes. Previously only the orderly `cider-quit` path killed the tunnel, so an abnormal disconnect (server crash, network drop) left the `ssh` subprocess as a zombie until Emacs exited.
+- Bound `nrepl-completed-requests` with a FIFO cap (`nrepl-completed-requests-max-size`, default 1000). The completed-request handler table previously grew unbounded for the lifetime of a connection; long-running sessions accumulated thousands of stale handler closures.
 
 ### Changes
 

lisp/nrepl-client.el

@@ -71,6 +71,7 @@
 (require 'seq)
 (require 'subr-x)
 (require 'cl-lib)
+(require 'queue)
 (require 'nrepl-dict)
 (require 'nrepl-bencode)
 (require 'tramp)
@@ -110,6 +111,24 @@ Setting this to nil disables the timeout functionality."
 When true some special buffers like the server buffer will be hidden."
   :type 'boolean)
 
+(defcustom nrepl-completed-requests-max-size 1000
+  "Maximum number of completed-request handlers retained per connection.
+
+nREPL servers occasionally send late messages for requests whose
+\"done\" status has already arrived (typically a sub-second window).
+We keep the recently-completed handlers around so those messages still
+get dispatched.  Without a bound, the table grew unbounded for the
+lifetime of the connection.
+
+Eviction is FIFO: when the table is full, the oldest entry is dropped
+to make room for the newest.  1000 is ample in practice -- requests
+complete in milliseconds, so this caps roughly a second of activity.
+Set to 0 to disable the cache entirely; late messages will then become
+log warnings."
+  :type 'integer
+  :safe #'integerp
+  :package-version '(cider . "1.22.0"))
+
 ;;; Buffer Local Declarations
 
 ;; These variables are used to track the state of nREPL connections
@@ -138,6 +157,10 @@ To be used for tooling calls (i.e. completion, eldoc, etc)")
 
 (defvar-local nrepl-completed-requests nil)
 
+(defvar-local nrepl--completed-requests-order nil
+  "FIFO of ids in `nrepl-completed-requests', used for bounded eviction.
+See `nrepl-completed-requests-max-size'.")
+
 (defvar-local nrepl-last-sync-response nil
   "Result of the last sync request.")
 
@@ -583,7 +606,8 @@ client buffer.  Return the newly created client process."
             nrepl-tunnel-buffer (when-let* ((tunnel (plist-get endpoint :tunnel)))
                                   (process-buffer tunnel))
             nrepl-pending-requests (make-hash-table :test 'equal)
-            nrepl-completed-requests (make-hash-table :test 'equal)))
+            nrepl-completed-requests (make-hash-table :test 'equal)
+            nrepl--completed-requests-order (queue-create)))
 
     (with-current-buffer client-buf
       (nrepl--init-client-sessions client-proc)
@@ -658,12 +682,22 @@ Used by the client to clean up session state on disconnect.")
 
 (defun nrepl--mark-id-completed (id)
   "Move ID from `nrepl-pending-requests' to `nrepl-completed-requests'.
-It is safe to call this function multiple times on the same ID."
-  ;; FIXME: This should go away eventually when we get rid of
-  ;; pending-request hash table
+The completed table is bounded by `nrepl-completed-requests-max-size';
+when the cap is reached the oldest entry is evicted FIFO.
+
+It is safe to call this function multiple times on the same ID -- the
+second call is a no-op because the entry has already been moved out of
+`nrepl-pending-requests'."
   (when-let* ((handler (gethash id nrepl-pending-requests)))
     (puthash id handler nrepl-completed-requests)
-    (remhash id nrepl-pending-requests)))
+    (remhash id nrepl-pending-requests)
+    (when nrepl--completed-requests-order
+      (queue-enqueue nrepl--completed-requests-order id)
+      (when (and (> nrepl-completed-requests-max-size 0)
+                 (> (queue-length nrepl--completed-requests-order)
+                    nrepl-completed-requests-max-size))
+        (remhash (queue-dequeue nrepl--completed-requests-order)
+                 nrepl-completed-requests)))))
 
 (defun nrepl-notify (msg type)
   "Handle a server notification with MSG and TYPE.

test/nrepl-client-tests.el

@@ -287,6 +287,44 @@
     (expect (nrepl--dispatch-response '(dict "id" "1" "value" "v"))
             :to-throw 'error)))
 
+(describe "nrepl--mark-id-completed cap"
+  :var (nrepl-pending-requests nrepl-completed-requests
+        nrepl--completed-requests-order
+        nrepl-completed-requests-max-size)
+  (before-each
+    (setq nrepl-pending-requests (make-hash-table :test 'equal)
+          nrepl-completed-requests (make-hash-table :test 'equal)
+          nrepl--completed-requests-order (queue-create)))
+
+  (cl-flet ((mark (id)
+              ;; A handler must exist in pending for the move to take place.
+              (puthash id #'ignore nrepl-pending-requests)
+              (nrepl--mark-id-completed id)))
+
+    (it "retains entries up to the configured cap"
+      (let ((nrepl-completed-requests-max-size 3))
+        (mark "1") (mark "2") (mark "3")
+        (expect (hash-table-count nrepl-completed-requests) :to-equal 3)
+        (dolist (id '("1" "2" "3"))
+          (expect (gethash id nrepl-completed-requests) :not :to-be nil))))
+
+    (it "evicts the oldest entry FIFO when over the cap"
+      (let ((nrepl-completed-requests-max-size 2))
+        (mark "1") (mark "2") (mark "3")
+        (expect (hash-table-count nrepl-completed-requests) :to-equal 2)
+        (expect (gethash "1" nrepl-completed-requests) :to-be nil)
+        (expect (gethash "2" nrepl-completed-requests) :not :to-be nil)
+        (expect (gethash "3" nrepl-completed-requests) :not :to-be nil)))
+
+    (it "treats max-size of 0 as unbounded"
+      ;; Documented as "disable the cache"; concretely the eviction
+      ;; branch is bypassed and the table grows freely.  In practice
+      ;; users would also need to clear the queue, but the queue itself
+      ;; is bounded by the producer rate, so this is fine.
+      (let ((nrepl-completed-requests-max-size 0))
+        (dotimes (n 5) (mark (number-to-string n)))
+        (expect (hash-table-count nrepl-completed-requests) :to-equal 5)))))
+
 (describe "nrepl-client-lifecycle"
   (it "start and stop nrepl client process"