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

REPL shouldn't auto-show giant outputs #40735

Closed
ericphanson opened this issue May 6, 2021 · 5 comments · Fixed by #53959
Closed

REPL shouldn't auto-show giant outputs #40735

ericphanson opened this issue May 6, 2021 · 5 comments · Fixed by #53959
Labels
display and printing Aesthetics and correctness of printed representations of objects.

Comments

@ericphanson
Copy link
Contributor

ericphanson commented May 6, 2021

copied from Slack:

Related to #40724, I wonder if there’s a way for the REPL to just refuse to print anything too big? Basically I never want 1M characters worth of stuff dumped into the repl, but I run into it all too often (table types without a good show method, deeply nested type parameters, long strings, etc). For each of those there’s a better individual fix but it kinda feels like a bug to me if the repl will just hang or even oom or crash if you show something that doesn’t have a good show method.

I think it should error ("Output too large to display in REPL") or something instead of hanging or trying to print something giant to the REPL.

@pfitzseb mentioned

the issue with that is that you need to buffer the stream internally
but Debugger.jl for example does that

edit: changed the title to "auto-show" instead of print. In my mind, the issue is that when you unexpectedly dump a huge amount of text to the REPL. If you're typing out print(...) then it's probably fine (there's at least some opting-in) but when it's an automatic display, then it's worse if it behaves badly or unexpectedly.

@mbauman mbauman added the display and printing Aesthetics and correctness of printed representations of objects. label May 6, 2021
@ericphanson
Copy link
Contributor Author

Sebastian pointed me to https://github.com/JuliaDebug/Debugger.jl/blob/master/src/limitio.jl and https://github.com/JuliaDebug/Debugger.jl/blob/master/src/printing.jl#L2-L20 and that it should probably be part of REPLDisplay.

Using just the LimitIO struct in display(d::REPLDisplay, mime::MIME"text/plain", x) via

diff --git a/stdlib/REPL/src/REPL.jl b/stdlib/REPL/src/REPL.jl
index 0c5a6c6267..3aa3eee2b0 100644
--- a/stdlib/REPL/src/REPL.jl
+++ b/stdlib/REPL/src/REPL.jl
@@ -239,6 +239,20 @@ function repl_backend_loop(backend::REPLBackend)
     return nothing
 end
 
+mutable struct LimitIO{IO_t <: IO} <: IO
+    io::IO_t
+    maxbytes::Int
+    n::Int # max bytes to write
+end
+LimitIO(io::IO, maxbytes) = LimitIO(io, maxbytes, 0) 
+
+struct LimitIOException <: Exception end
+
+function Base.write(io::LimitIO, v::UInt8)
+    io.n > io.maxbytes && throw(LimitIOException())
+    io.n += write(io.io, v)
+end
+
 struct REPLDisplay{R<:AbstractREPL} <: AbstractDisplay
     repl::R
 end
@@ -254,7 +268,7 @@ function display(d::REPLDisplay, mime::MIME"text/plain", x)
             # this can override the :limit property set initially
             io = foldl(IOContext, d.repl.options.iocontext, init=io)
         end
-        show(io, mime, x[])
+        show(LimitIO(io, 100_000), mime, x[])
         println(io)
     end
     return nothing

I get what seems like nice behavior to me: it will print the first 100k bytes of data from show and then display an error if it's too long. But if you print to stdout directly, it doesn't limit anything.

@ericphanson ericphanson changed the title REPL shouldn't print giant outputs REPL shouldn't auto-show giant outputs May 6, 2021
@stevengj
Copy link
Member

We already have a :limit property for IOContext. Individual show methods should really truncate the output if get(io, :limit, false) == true. Arguably, any show method that ignores this flag for arbitrarily large outputs is a bug.

For example, the String and showtype output could be updated to respect this flag.

@ericphanson
Copy link
Contributor Author

We already have a :limit property for IOContext. Individual show methods should really truncate the output if get(io, :limit, false) == true. Arguably, any show method that ignores this flag for arbitrarily large outputs is a bug.

For example, the String and showtype output could be updated to respect this flag.

Agreed with that! However, I think it's a worse bug if an invalid show method can hang/crash your Julia session. Usually in Julia if you aren't doing something unsafe and aren't doing some terrible piracy, then if you define a bad method you'll just get an exception. But here you can make the mistake of showing a giant string or table (or writing a bad show method for such a thing) and instead an exception you can get OOM'd or your session hanging for a very long time etc.

So I think the problematic methods should be fixed by respecting the limit in whatever way is appropriate for the thing being shown, but also the REPL itself should refuse to show something that's way too big.

@timholy
Copy link
Member

timholy commented Jul 25, 2022

Cthulhu has a width-limiting IO object that I've long wondered about making more widely available. Limiting by size of output is a feature that arguably belongs more to the IO object than the object being displayed, although of course you can do "smarter" things (e.g., informative summaries) by dispatching on the object itself.

@KristofferC
Copy link
Member

Debugger has https://github.com/JuliaDebug/Debugger.jl/blob/master/src/limitio.jl which has the same purpose but is much more rudimentary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants