fix: apply cider-print-quota to load-file requests#4113
fix: apply cider-print-quota to load-file requests#4113mmustafasenoglu wants to merge 2 commits into
Conversation
The load-file op did not include print middleware parameters (nrepl.middleware.print/quota, etc.), so cider-print-quota was not enforced when evaluating buffers via cider-eval-buffer / cider-load-buffer. Large evaluation results were sent in full to the echo area, which could freeze or crash Emacs. Add the standard cider--nrepl-print-request-plist to the load-file request so that cider-print-quota (default 1 MB) is honored, matching the behavior of eval requests. Fixes clojure-emacs#3052 Signed-off-by: Mustafa Senoglu <mmustafasenoglu0@gmail.com>
bbatsov
left a comment
There was a problem hiding this comment.
Good catch, this is a real gap. load-file sends no print params, so the last form's value comes back unbounded (I checked against a live server: a modest map came back at 82KB, and it can be much bigger), while eval bounds it via the quota. I confirmed load-file does honor the quota when we send it, so the approach is sound.
One change I'd want before merging (inline), and I'd keep #3052 open rather than close it. That issue is really about making the result destination configurable (send it to the REPL, or nowhere), which this doesn't add. Bounding the size fixes the crash, which is worth doing on its own, but it's a different thing from what the reporter asked for, so maybe reword to "Addresses #3052". A test asserting the load-file request carries the quota would be good too.
| "file-path" ,file-path | ||
| "file-name" ,file-name) | ||
| "file-name" ,file-name | ||
| ,@(cider--nrepl-print-request-plist)) |
There was a problem hiding this comment.
I'd reach for cider--nrepl-pr-request-plist here instead. The eval commands that echo their result to the minibuffer (cider-eval-last-sexp, cider-eval-region) use the pr variant, and it carries the quota too, so it fixes the crash while keeping the echoed result single-line and consistent with eval. cider--nrepl-print-request-plist would switch load-file's result to pretty-printed and streamed, which is a behavior change I don't think we want on this path.
| (provide 'cider-client) | ||
|
|
||
| ;;; cider-client.el ends here | ||
|
|
There was a problem hiding this comment.
Stray trailing newline, drop this.
- → (pr variant keeps echoed result single-line, consistent with eval) - Remove stray trailing newline at end of file Requested by @bbatsov
|
Done. Thanks for the review!
|
|
Thanks, the code looks good now. One last thing before I merge: could you change "Fixes #3052" to "Addresses #3052" in the description? That issue is really about making the result destination configurable (send it to the REPL, or nowhere), which this doesn't add, so I'd rather it not auto-close on merge. After that this is good to go. |
Problem
When using
cider-eval-buffer/cider-load-buffer, the evaluation result of the last form is displayed in the echo area viacider--display-interactive-eval-result. If this result is large (e.g., a deep map or huge string), Emacs freezes or crashes because the full result is sent to the echo area.While
cider-print-quota(default 1 MB) truncates results forevalrequests, theload-fileop used bycider-load-bufferdid not include the print middleware parameters, socider-print-quotawas not enforced.Fix
Add
cider--nrepl-print-request-plistto theload-filerequest incider-load-file-request, so thatcider-print-quota,cider-print-fn, and other print settings are honored — matching the behavior ofevalrequests.Before:
After:
Why this works
cider--nrepl-print-request-plistalready provides all the standard print middleware parameters:nrepl.middleware.print/quota→cider-print-quota(default 1 MB)nrepl.middleware.print/print→cider-print-fnnrepl.middleware.print/stream?→ streaming flagnrepl.middleware.print/buffer-size→cider-print-buffer-sizenrepl.middleware.print/options→ other print optionsThese parameters were already being sent for
evalrequests but were missing from theload-filerequest path.Related issue
Fixes #3052