Plug request-id leaks in raw nREPL response handlers#3894
Merged
Conversation
Five raw `(lambda (response) ...)' callbacks passed to `cider-nrepl-send-request' never called `nrepl--mark-id-completed', so their request ids accumulated in the connection's `nrepl-pending-requests' hash table for the lifetime of the session. The dispatcher (`nrepl--dispatch-response') falls back to the completed table when a pending entry isn't found, so this is silent: the leak does not cause functional problems, just a slow growth of memory and stale-id lookups on every response. Sites: - cider-test.el cider/test-stacktrace - cider-test.el cider/test-var-query (test runner) - cider-eval.el cider/analyze-last-stacktrace - cider-ns.el cider/ns-reload - cider-repl.el cider/get-state (fire-and-forget) Each `done'-status branch now calls `nrepl--mark-id-completed'. The fire-and-forget `cider/get-state' caller switches to `cider-nrepl-send-unhandled-request', which marks the id complete immediately and lets the global state-handler hook do the per-response work. The other four can't cleanly migrate to `nrepl-make-eval-handler' because they consume non-eval response slots (`results', `summary', `phase', `class', `reloading', etc.). Found during a wider audit of `nrepl-send-request' callbacks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found during a wider audit of
nrepl-send-requestcallbacks (the same audit that produced #3893).Five raw
(lambda (response) ...)callbacks passed tocider-nrepl-send-requestnever callednrepl--mark-id-completed, so their request ids accumulated in the connection'snrepl-pending-requestshash for the lifetime of the session. The dispatcher falls back to the completed table when a pending entry isn't found, so this was silent: not a functional bug, just a slow leak on every response.cider-test.elcider/test-stacktracecider-test.elcider/test-var-query(the test runner)cider-eval.elcider/analyze-last-stacktracecider-ns.elcider/ns-reloadcider-repl.elcider/get-state(fire-and-forget)Four of them now call
nrepl--mark-id-completedin theirdone-status branch. The fire-and-forgetcider/get-statesite switches tocider-nrepl-send-unhandled-request, which marks the id complete immediately and lets the globalcider-repl--state-handlerhook do the per-response work.These don't migrate cleanly to
nrepl-make-eval-handlerbecause they consume non-eval response slots (results,summary,phase,class,reloading, ...) -- adding a "raw fallback" slot to the eval-handler abstraction for five sites isn't worth it. One-linemark-id-completedcalls per site is.Full suite still 542/544 pass (3893 is in flight on a separate branch).