Skip to content

Commit 3190a37

Browse files
committed
Stop response-handler errors from poisoning the nREPL response queue
`nrepl-client-filter' decodes a batch of responses from the wire and walks them in a `while' loop, dispatching each in turn. A demoted- errors wrapper guarded the global `nrepl-response-handler-functions' hook but stopped there -- the per-request `nrepl--dispatch-response' call ran unprotected. If that call signaled (because of a buggy callback, or because no handler was registered for the response's id), the loop aborted and any later decoded responses sitting in the queue were dropped. Two changes: - Wrap `nrepl--dispatch-response' in `with-demoted-errors' too. Handler bugs are logged but no longer corrupt the response stream. - Soften `nrepl--dispatch-response''s no-callback case from `error' to `message'. The dispatcher cannot do anything actionable with an unknown id; logging is sufficient. Adds two specs in test/nrepl-client-tests.el covering the soft no-handler path and locking in the dispatcher's "errors propagate unless the filter wraps you" contract.
1 parent 0ff2841 commit 3190a37

3 files changed

Lines changed: 35 additions & 4 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
- `cider--resolve-command` now actually verifies command presence on remote hosts via `executable-find`'s remote search, instead of unconditionally trusting the user-supplied command. A missing tool on the remote side now surfaces immediately during jack-in instead of as a server-startup failure later.
2323
- `nrepl--ssh-tunnel-connect` no longer routes its `ssh` invocation through a shell. The tunnel command is now spawned via `start-process` with an explicit arg list, eliminating shell-quoting hazards in user/host components.
2424
- 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.
25+
- 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.
2526

2627
### Changes
2728

lisp/nrepl-client.el

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,11 @@ functions are demoted to messages, so that they don't prevent the
282282
callbacks from running.")
283283

284284
(defun nrepl-client-filter (proc string)
285-
"Decode message(s) from PROC contained in STRING and dispatch them."
285+
"Decode message(s) from PROC contained in STRING and dispatch them.
286+
Errors raised by individual response handlers (or by the global
287+
`nrepl-response-handler-functions' hook) are demoted to messages so that
288+
a single misbehaving callback cannot poison the response queue and drop
289+
later responses on the floor."
286290
(let ((string-q (process-get proc :string-q)))
287291
(queue-enqueue string-q string)
288292
;; Start decoding only if the last letter is 'e'
@@ -294,20 +298,26 @@ callbacks from running.")
294298
(let ((response (queue-dequeue response-q)))
295299
(with-demoted-errors "Error in one of the `nrepl-response-handler-functions': %s"
296300
(run-hook-with-args 'nrepl-response-handler-functions response))
297-
(nrepl--dispatch-response response))))))))
301+
(with-demoted-errors "Error in nREPL response dispatch: %s"
302+
(nrepl--dispatch-response response)))))))))
298303

299304
(defun nrepl--dispatch-response (response)
300305
"Dispatch the RESPONSE to associated callback.
301306
First we check the callbacks of pending requests. If no callback was found,
302307
we check the completed requests, since responses could be received even for
303-
older requests with \"done\" status."
308+
older requests with \"done\" status.
309+
310+
When neither table has a callback for the response's id, log a message
311+
instead of raising an error: there is nothing actionable for the
312+
dispatcher to do, and signaling here used to abort processing of any
313+
later responses sitting in the queue."
304314
(nrepl-dbind-response response (id)
305315
(nrepl-log-message response 'response)
306316
(let ((callback (or (gethash id nrepl-pending-requests)
307317
(gethash id nrepl-completed-requests))))
308318
(if callback
309319
(funcall callback response)
310-
(error "[nREPL] No response handler with id %s found for %s" id (buffer-name))))))
320+
(message "[nREPL] No response handler with id %s found for %s" id (buffer-name))))))
311321

312322
(defun nrepl-client-sentinel (process message)
313323
"Handle sentinel events from PROCESS.

test/nrepl-client-tests.el

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,26 @@
267267
(expect received-body :to-equal "hello")
268268
(expect received-type :to-equal '("text/plain" ())))))
269269

270+
(describe "nrepl--dispatch-response"
271+
:var (nrepl-pending-requests nrepl-completed-requests)
272+
(before-each
273+
(setq nrepl-pending-requests (make-hash-table :test 'equal)
274+
nrepl-completed-requests (make-hash-table :test 'equal)))
275+
276+
(it "logs a message instead of erroring when no callback is registered"
277+
(spy-on 'message)
278+
(nrepl--dispatch-response '(dict "id" "404" "value" "anything"))
279+
(expect 'message :to-have-been-called))
280+
281+
(it "does not raise even when the registered callback throws"
282+
(puthash "1" (lambda (_) (error "boom!")) nrepl-pending-requests)
283+
;; Demoted-errors lives in `nrepl-client-filter', so a direct call
284+
;; to the dispatcher with a throwing callback DOES propagate. This
285+
;; spec just locks the contract: the dispatcher itself doesn't add
286+
;; its own protection -- protection is at the filter layer.
287+
(expect (nrepl--dispatch-response '(dict "id" "1" "value" "v"))
288+
:to-throw 'error)))
289+
270290
(describe "nrepl-client-lifecycle"
271291
(it "start and stop nrepl client process"
272292

0 commit comments

Comments
 (0)