clojure-emacs / cider-nrepl

A collection of nREPL middleware to enhance Clojure editors with common functionality like definition lookup, code completion, etc.
https://docs.cider.mx/cider-nrepl
677 stars 176 forks source link

`middleware.refresh` / Haystack can get stuck printing #870

Open vemv opened 5 months ago

vemv commented 5 months ago

My cider-nrepl got stuck on a refresh op. A thread keeps working indefinitely.

This is the stracktrace grabbed from yourkit https://gist.github.com/vemv/73bd4908dcfe4ea8983d7abb57af9f29

Looks like we could bind some *print-,,, options somewhere.

vemv commented 5 months ago

@alexander-yakushev just in case - does the stacktrace ring a bell?

The fix seems easy anyway but I only started getting these after locally installing cider-nrepl master, so perhaps there's some problem that slipped by.

alexander-yakushev commented 5 months ago

The stacktrace only contains clojure.pprint entries which the new stuff in Orchard/cider-nrepl doesn't use. So this is unlikely to be caused by the latest changes. Nothing referring to orchard.print in the stack.

alexander-yakushev commented 5 months ago

A good fix to avoid these kinds of situations would be, for example, to replace StringWriter with TruncatingStringWriter here: https://github.com/clojure-emacs/haystack/blob/a6c77ae051cb9506d53ab96abdd63ae6bfc5ca44/src/haystack/analyzer.clj#L369. Can set the limits to something arbitrary big.

No, that would not help, because it will only prevent the stringwriter to grow indefinitely, but the pprint endless loop will keep going because it is not cooperating. Is it required to use clojure.pprint there? Can a custom cooperating printer be an adequate substitute?

vemv commented 5 months ago

Thanks for the eyeing!

Any of the solutions you've recently used/introduced would sound good to me.

IMO for Haystack stuff, we should quite agressively trim stuff beyond 1 screenful of text. Any more that that, I'd encourage users to inspect the exception.

alexander-yakushev commented 5 months ago

Can you help me understand where that output was going to go? Was it to be printed to the REPL output, or shown as a separate buffer? In any case, IIUC, that pprint is meant for the nested structure that Haystack produces. You can't really avoid pretty-printing that because of all the nesting and stuff, it would be a mess otherwise.

So, a simple (but not 100% bulletproof) solution would be to (binding [*print-length* 100, *print-level* 5] ...) around invoking print-fn on the line that I linked to.

Overall, by skimming through that namespace, I'd say I'm not fond of how liberally it prints through user data that can potentially be big/infinite. Or, maybe, it believes that nrepl.middleware.print will truncate such data, I haven't figured out the chain of responsibility yet.

alexander-yakushev commented 5 months ago

In any case, I believe that all of CIDER (and possibly nrepl) deserves going through all places where something unknown is printed, and making those places more rigorous and robust to arbitrary data.

vemv commented 5 months ago

Can you help me understand where that output was going to go? Was it to be printed to the REPL output, or shown as a separate buffer?

Most times it goes to the *cider-error* buffer. Although CIDER performs other requests which aren't necessarily even rendered.

Or, maybe, it believes that nrepl.middleware.print will truncate such data

Maybe that would be a good and standard-looking approach - ditch haystack.analyzer/pprint and use nrepl.middleware.print instead, which has truncation and user-supplied options that CIDER forwards on most (every?) request.

In any case, I believe that all of CIDER (and possibly nrepl) deserves going through all places where something unknown is printed, and making those places more rigorous and robust to arbitrary data.

Sounds reasonable. Out of caution, if you'd be interested in improving that area, please as a first pass simply identify the bad areas in a GH issue - perhaps some case deserves a different treatment.

vemv commented 4 months ago

In any case, I believe that all of CIDER (and possibly nrepl) deserves going through all places where something unknown is printed, and making those places more rigorous and robust to arbitrary data.

Is this still on your radar these days?

alexander-yakushev commented 4 months ago

Currently doing other things around nREPL, so you can take it.