Repository navigation
Fix how neat handles late output, errors, stdin and sessions - #2
Merged
Merged
Conversation
nil is ambiguous in elisp (empty list, empty dict, or a missing value), and quietly turning it into `le' put garbage on the wire. Vectors now encode as lists, so there's still a way to send an empty list, or a list of lists that would otherwise look like an alist. Now that encoding can fail on ordinary input, neat-send encodes before registering the callback, so a message that never went out leaves no pending callback or log entry behind.
A non-dict message or a string status used to raise in dispatch and leave the rest of the chunk stuck until the next filter call. Anything that isn't a dict is now skipped, a status sent as a plain string is treated as a list of one, and any other non-list status is ignored. Malformed bencode drops the connection instead of clearing the buffer and carrying on mid-message, which could only mess up whatever came next. The dict check dispatch relies on is neat-bencode's own, now public as neat-bencode-dict-p.
The fake reply neat sent when a connection closed reused nREPL's `interrupted' status and put an err without a newline in the REPL, so it looked like the server had said something. It now carries a neat-only `connection-closed' status and no text, and the first REPL prompt no longer goes up when that's all the clone gets back. The sync helpers now share neat-client--request-sync, which returns whatever arrived and, when it gives up, removes the callback instead of leaving it pending forever.
Output for a request that already got its done (say, a future that prints after the eval returned) and output with no id at all used to vanish. Such messages now go to neat-unhandled-message-functions, and the REPL prints their out and err above the waiting prompt. That text goes in through comint-output-filter itself, with the process mark moved to the prompt for the duration, so filters, ANSI colors and carriage motion work as for any other output. It keeps its own ANSI color state, as the one streamed output uses points past the prompt, and a chunk that ends in a carriage return gets its line overwritten by the next one. When comint-move-point-for-output is set, point goes to the input, not onto the prompt. The connection-closed marker goes above a waiting prompt too, and a new prompt always starts on a line of its own, so reconnecting in the same buffer doesn't put it on the dead prompt's line.
Basilisp and jank put the full stacktrace in both err and ex, so every error showed up twice. ex is now held until done and shown only if nothing came on err, which also keeps nREPL's bare exception class from trailing its own error message.
A source-buffer eval sent with an explicit ns reports that ns back, but the server only binds it for the one eval. Taking the ns from every response left the prompt claiming a namespace the REPL wasn't in. The prompt now follows a reply's ns only when the request named none, which still covers REPL input and source-buffer evals that move the session with (ns ...) or (in-ns ...).
They went out without one, and jank answers that with namespace-not-found or a bare error. The ns now comes from neat-buffer-ns-function, falling back to the namespace the REPL last reported, which is kept on the connection for this. A namespace the server hasn't loaded yet (one set from .dir-locals in a fresh buffer, say) may get turned down in turn, so a request whose ns comes back namespace-not-found, or a bare error, with no result goes out once more without it. Eldoc gets the same through the new neat-lookup-async, and now also calls back with nil when there's no doc, so eldoc isn't left waiting on it.
The describe reply was stored and never read, although the design doc said completion and eldoc used it. Basilisp lists no completions or lookup op, and Babashka and jank have no interrupt, so those features fired at ops that weren't there. neat-op-supported-p now decides. Completion, eldoc and xref step aside, while the doc lookup and interrupt commands say the server can't do it. A done that comes on its own no longer wipes out the stored ops. Interrupt replies are no longer ignored either: unknown-op, session-idle, session-ephemeral, interrupt-id-mismatch and plain errors all get reported.
Babashka handles one request at a time per connection, so a completions request sent behind a long eval made Emacs freeze for the whole sync timeout on every try, and eldoc lookups piled up and all answered once the eval was over. Holding off whenever an eval is pending would cost nREPL, which answers tooling ops mid-eval, its completion for as long as an eval runs, and for good if a done never comes. So the connection learns instead. It tracks evals in flight, and a tooling request that times out while one is running marks the connection as stuck (neat-tooling-stalled-p). Completion and eldoc skip their requests while it is, except for one test request every neat-tooling-probe-interval seconds. The mark goes once the evals are done or a request is answered mid-eval. Eldoc lookups now time out too, through neat-lookup-async, and only one goes out at a time per connection; one that fails to go out doesn't keep the next ones from going.
An eval in a missing namespace comes back as nothing but namespace-not-found/done/error, so a typo in neat-ns just printed a new prompt, and without a REPL buffer nothing showed at all. namespace-not-found, unknown-session, unknown-op and a bare error now render as a ;; line in the REPL or an echo-area message. A bare error after the request already showed an err or ex gets no line of its own, in the REPL or the echo area, which now also shows an ex when nothing came on err. The missing namespace never becomes the prompt's. Closing the connection's session now clears it from the connection, and an unknown-session reply drops the dead session, and the namespace it was in, then offers to clone a new one rather than failing every eval after it.
There was no way to give a reading eval end-of-file, the reply always went to the connection's session rather than the eval's, and a need-input from a source-buffer eval with no REPL buffer was never answered, so the eval got stuck. The stdin prompt now takes C-c C-d for EOF, replies in the asking eval's session, also comes up without a REPL, and on C-g interrupts that eval, reporting an interrupt that didn't happen, or sends EOF when the server can't interrupt. The REPL and the no-REPL path share neat-repl--answer-status for the statuses that need the user.
Quitting the REPL or killing its buffer only dropped the socket, so every REPL left a session behind on the server. Both now send close for the session first, when the server has the op. Quitting waits briefly for the reply, through the new neat-close-session-sync. Killing the buffer doesn't, so a busy or stuck server can't hold it up.
Nothing more can arrive for a request the server has no session for, but if the reply leaves done off, the callback stays pending and the eval stays in flight for good: no REPL prompt, and completion and eldoc stay off. Such replies now get a done added.
Checking servers against each other is proof's job now, so the suite drops its table of implementations and runs against nREPL alone. It now covers what only a real wire shows: REPL rendering, the stdin round trip, interrupts, error statuses and late output. README and the design doc point server authors at proof.
comint stopped dropping repeated prompts back in Emacs 27, so a source-buffer eval that finished while the REPL sat at a prompt stacked a second prompt on the same line, and any result went after the waiting prompt. That also left the prompt marker mid-line, so late output landed in the wrong place. Results of source-buffer evals now go above a waiting prompt, only input typed into the REPL gets a new prompt at done, and a prompt never goes in while another is waiting. When such an eval moves the session to another namespace, the waiting prompt gets updated to match.
quit isn't an error, so C-g at the clone-a-new-session prompt went straight past the callback guard and skipped the cleanup after it. The request stayed pending and looked like an eval in flight, which kept completion and eldoc off until the connection closed. Finished requests are now removed before the callback runs, and C-g at that prompt just means no.
C-g during the short wait for the close reply went past ignore-errors, so the REPL never disconnected and the connection stayed up with its session already cleared. The shared sync helper also skipped removing its callback on C-g. Both cleanups now run from unwind-protect, while a wait that's cut short (a quit, or a completion UI's while-no-input) still isn't treated as the server being stuck.
need-input can arrive while the user is in the minibuffer, and without recursive minibuffers read-from-minibuffer just signals. No stdin went out and the eval waited forever. Both prompts that come up from the process filter now allow a recursive minibuffer.
The autoloads put neat-repl--handle-disconnect and neat--connections-buffer-refresh on neat-disconnect-functions before either function exists, so code using neat-client on its own got a void-function error, turned into a message, every time a connection closed. Both hooks now go on when their file loads, as the unhandled-message hook does, instead of from the autoloads.
bbatsov
force-pushed
the
protocol-fixes
branch
from
October 2, 2026 10:01
cb5f54c to
0061873
Compare
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
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.
This covers the parts of the protocol neat used to get wrong or just ignore: late and id-less output, error statuses that come without an err, unknown sessions, stdin EOF, ops the server doesn't advertise, and the session left behind on quit. Results of source-buffer evals now go above the REPL prompt (like in CIDER) instead of after it.
Completion and eldoc back off once a tooling request times out while an eval is running, and try again every few seconds. nREPL answers them mid-eval, so it never gets there, while on Babashka a long eval now costs a short wait every few seconds instead of freezing Emacs on every completion. The integration suite runs against nREPL alone now, as checking the servers themselves is proof's job.