Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

middleware.refresh / Haystack can get stuck printing #870

Open
vemv opened this issue May 1, 2024 · 9 comments
Open

middleware.refresh / Haystack can get stuck printing #870

vemv opened this issue May 1, 2024 · 9 comments

Comments

@vemv
Copy link
Member

vemv commented May 1, 2024

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
Copy link
Member Author

vemv commented May 2, 2024

@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
Copy link
Member

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
Copy link
Member

alexander-yakushev commented May 2, 2024

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
Copy link
Member Author

vemv commented May 2, 2024

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
Copy link
Member

alexander-yakushev commented May 2, 2024

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
Copy link
Member

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
Copy link
Member Author

vemv commented May 2, 2024

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
Copy link
Member Author

vemv commented May 21, 2024

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants