[Fix #3909] Simplify friendly-session matching#3913
Merged
Conversation
`cider--sesman-friendly-session-p` decides whether an unlinked source buffer can attach to an existing CIDER session. Three bugs made it under-match on some projects (CLJS buffers staying `[not connected]` after a successful jack-in being the obvious symptom): * Empty classpath / ns-list responses were cached as nil, which is indistinguishable from "not yet cached". The matcher re-fetched on every check, wasting an nREPL round trip and never getting a useful cached value. Cache through a new `cider--cached-or-fetch` helper that uses a tagged-cons sentinel. * classpath-roots was matched with `string-prefix-p`, which has no directory boundary -- `/foo/bar` would match `/foo/barber/x.clj`. Use `file-in-directory-p` for both the direct and translated-path checks. * When the classpath / classpath-roots checks miss (e.g. the cider-nrepl classpath op isn't available, or the connection hasn't populated it yet), fall back to the connection's `nrepl-project-dir` before giving up. The debug listing reports this fallback too.
The friendly-session matcher used to fetch classpath and ns-list lazily on first sesman call, then cache the result on the REPL process. This had two downsides: * The caches confused empty results with "not cached" (the bug behind #3909, just fixed). The sentinel workaround introduced in the previous commit is no longer needed if we populate eagerly: an unpopulated cache is just nil, and the matcher falls through to project-dir / ns-form fallbacks. * The matcher had a slow non-cached path that blocked sesman calls on a nREPL round trip. The comment in `cider-repl-handle-mark-evals' spelled out that we couldn't use it for font-lock without surprising latency. Move the population to `cider--connected-handler' via a new `cider--precompute-friendly-session-cache' helper, drop the `cider--cached-or-fetch' sentinel helper, and shrink the matcher to a pure path comparison. While here, also short-circuit the matcher when `cider-default-session' is set: only the chosen session is considered friendly. This keeps the matcher consistent with `cider-repls', which already bypasses sesman dispatch when a default session is pinned.
bbatsov
changed the title
* Soft-fall-through when `cider-default-session' names a session that no longer exists. `cider-repls' already handles this case gracefully (message + fall through); the friendly-session matcher now matches. * Move the precompute call above `cider-enable-on-existing-clojure-buffers' to close the small window where mode-enable can trigger a sesman query against an empty cache. * Add buttercup specs for `cider--sesman-friendly-session-p' covering the default-session short-circuit (including the soft-fallthrough case), the ancillary-buffer branch, classpath matching, the `file-in-directory-p' boundary fix, and the project-dir fallback.
* Use `string-suffix-p` instead of an anchored regex for JAR detection. * Use `delq nil` instead of `seq-remove #'null', matching codebase style. * Tighten the connect-handler comment to one sentence. * Soften the matcher's "never blocks" docstring claim (the ns/buffer fallbacks can still hit the REPL). * Explain why classpath uses `string-prefix-p` while classpath-roots uses `file-in-directory-p` -- the two are intentionally asymmetric.
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.
Fixes #3909.
cider--sesman-friendly-session-pis the function that decides whether an unlinked source buffer can attach to an existing CIDER session. Three bugs made it under-match on some projects (CLJS buffers staying[not connected]after a successful jack-in being the obvious symptom), and the function was generally heavier than it needs to be.This PR is two commits:
1. Make friendly-session matching more robust (8d3e88bd) - the minimum fix for #3909:
nil, indistinguishable from "not yet cached" - the matcher re-fetched on every sesman call. Fixed with a tagged-cons sentinel.string-prefix-p(so/foo/barwould match/foo/barber/x.clj). Switched tofile-in-directory-pfor proper directory boundary.cider/classpathop isn't available, as is common for CLJS-only piggieback connections), fall back to the connection'snrepl-project-dirbefore giving up.2. Precompute friendly-session caches at connect time (ba1401c8) - structural simplification:
The lazy caching was load-bearing for the original bug. If we populate eagerly at connect time, the sentinel disappears, the matcher becomes a pure path comparison, and the "slow non-cached path" warned about in `cider-repl-handle-mark-evals' goes away entirely.
While here, also short-circuit the matcher when
cider-default-sessionis set: only the pinned session is considered friendly. This keeps the matcher consistent withcider-repls, which already bypasses sesman dispatch in that case.I considered keeping just commit 1 (the minimal fix), but the second commit drops more code than it adds and removes a class of bugs around cache semantics. Happy to split into separate PRs if preferred.