cider Atom print-method bricking shadow-cljs UI Inspect

[thheller]

I was debugging a problem as shadow-cljs user reported, and while trying to debug that I noticed that the shadow-cljs UI inspect wasn't working or very sluggish, frequently running into OutOfMemoryError. The cause is this bit of code

cider-nrepl/src/cider/nrepl/print_method.clj

Lines 37 to 40 in 247071b

	(def-print-method Atom c
	"#atom["
	(pr-str @c)
	(format " 0x%x]" (System/identityHashCode c)))

The pr-str being the problem here.

A short side note on how the problem manifests: The shadow-cljs UI Inspect lets you inspect objects of any size, so for example the full shadow-cljs runtime state, which with a build running can easily reach gigabytes when printed. It does so by using a LimitWriter, i.e. a Writer that stops after a certain amount of bytes. This is exactly to prevent large objects blowing everything up, or making everything slow, when a Human could never look at the value in full anyways.

To try you can run shadow-cljs clj-repl then (tap> shadow.cljs.devtools.server.runtime/instance-ref). With the cider-nrepl middleware installed this will be very very slow in the UI, without it is snappy. The UI is at http://localhost:9630/inspect.

The fix here would be to not use pr-str, but rather delegate the writer to print the object into the handled stream. The macro abstracts this away unfortunately in a way that basically requires rewriting the entire macro. The gist is to call (print-method @c writer), i.e. the writer that gets passed to the print-method in the first place. It is generally not a good idea to start one print from another, as you are supposed to use the supplied Writer.

I realize this is a bit specific to shadow-cljs and its LimitWriter, but I'd rather not do what is documented here

(def ^:dynamic *pretty-objects*
  "If true, cider prettifies some object descriptions.
  For instance, instead of printing functions as
      #object[clojure.core$_PLUS_ 0x4e648e99 \"clojure.core$_PLUS_@4e648e99\"]
  they are printed as
      #function[clojure.core/+]

  To disable this feature, do
      (alter-var-root #'cider.nrepl.print-method/*pretty-objects* not)"
  true)

as that would interfere with what a cider user might prefer. I'd also rather not start being defensive in the print mechanism for Inspect by binding this var.

It would be nice if the two uses of pr-str in that file, get changed to properly delegate to the print-method with the supplied Writer, as this is globally installed and affects everything.

[vemv]

Thanks for the report!

Rebinding *pretty-objects* seems pretty innocous and intended usage - using Clojure's original printing seems to be a very reasonable thing to want outside a repl.

Could you expand on why you would not want to create a simple (binding for this var?

[vemv]

It would be nice if the two uses of pr-str in that file, get changed to properly delegate to the print-method with the supplied Writer, as this is globally installed and affects everything.

(I'll look into this)

[thheller]

Yes, exactly. I want the default Clojure original printing. cider installs this globally and overwrites that default though.

I don't want binding because shadow-cljs does not use cider-nrepl itself, so it is not dependent on it. I also use Cursive, so don't use cider-nrepl at all and thus have never noticed myself. Fixing problems that have to check "is this library loaded?" first, seems like a bad way to work. I can't just access the var after all, I have to look it up by name first.

[vemv]

resolve and with-bindings seem simple, data-driven transformations though.

Assume we fixed this. What if the user was running an older cider-nrepl?

[thheller]

Note this isn't the first time shadow-cljs had to work arround cider "overreaching" with its code.

I might still add that binding regardless, or I might just add my own print-method. Mucking with global default things like this is bad form though.

[vemv]

I'm not saying it isn't, but it's what it's been there for many years so we have to tread carefully.

With a bit of luck, CIDER could only redefine these within the extent of repl evaluations, and perhaps a few other operations.

Please let us know how the binding approach goes. We'll try to improve things, regardless.

[vemv]

Hi @thheller I gave this task a serious shot, without luck.

It's hard to change the default and then to rebind it to true during specific nrepl ops.

Basically it doesn't work (I guess the dynamic binding gets lost in some step across the stack), and I'm not sure that it's worth further investigating atm.

How have you worked around this in the meantime, Shadow side?

[thheller]

No, I have no intention of working arround this on the shadow-cljs side. I guess the number of people that tap a huge (circular) atom, use the Inspect UI and do so while using cider is small enough to not matter.

There is also no need to change the binding in any way. I outlined how to fix it, all you need to remove is the pr-str and instead call (print-method @c the-writer-that-is-abstracted-away-by-the-def-print-method-macro). All you need is restore access to the writer the macro hides. That would keep using the described LimitWriter, bail at that limit and the issue is gone. pr-str is the only issue here from my side, since that creates a writer with its own StringBuilder, which eventually runs out of memory.

[cichli]

Could we remove that namespace completely in lieu of nrepl pretty printing support? If a client wants stuff to be pretty printed then they can ask for it explicitly.

I agree that it's really bad form to mess with print-method for types we don't define ourselves – regardless of the performance issues it causes in this case.

[bbatsov]

That might be a good idea. I see it was added a long time ago to address fairly trivial things 8e79ee6

 Abbreviate printing of AFunction and MultiFn objects

Instead of printing as
  #object[clojure.core$_PLUS_ 0x4e648e99 "clojure.core$_PLUS_@4e648e99"]
they are now printed as
  #function[clojure.core/+]

Instead of printing as
  #object[clojure.lang.MultiFn 0x4e648e99 "clojure.lang.MultiFn@0x4e648e99"]
they are now printed as
  #multifn[print-method 0x3f0cd5b4]

Disable this with
    (alter-var-root #'cider.nrepl.print-method/*pretty-objects* not)

[bbatsov]

For extra context - this legacy namespace predates the improvements made to nREPL's printing a few years ago, so it seems like an obsolete hack. I have to admit I had totally forgotten about it.