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

Streaming values #117

Closed
bbatsov opened this Issue Jan 10, 2019 · 7 comments

Comments

Projects
None yet
4 participants
@bbatsov
Copy link
Contributor

bbatsov commented Jan 10, 2019

That's a follow-up to a conversation I had with @brandonbloom brandonbloom/fipp#55

Generally I've never had issues with values being printed in pr-values, but he does raise a valid point. I wonder if we can somehow stream the values the way we do it for out and err without breaking backward compatibility.

That won't be an issue for the new rich printer, but it'd be nice if we improve the situation for older clients as well. I don't really have any ideas here, but I hope that @cgrand might have something up his sleeve.

I'm not sure how big of a problem is this in practice, as you should still be able to interrupt an infinite print, but you also have to realize this before the server runs out of memory.

@bbatsov bbatsov added the Enhancement label Jan 10, 2019

@cgrand

This comment has been minimized.

Copy link
Contributor

cgrand commented Jan 10, 2019

I'm afraid that streaming would not fix the issue as you just move the DoS around: you don't die of OoM any more but you saturate the connection and/or the client rendering.

@cichli

This comment has been minimized.

Copy link
Member

cichli commented Jan 10, 2019

I think this was the killer feature of cider-nrepl's pretty printing (and in fact my main motivation for developing it). To me this is about network transparency – when I'm using a REPL I just expect values to be printed incrementally, and the evaluation to be cancellable while printing is happening.

I'm working on this branch which repurposes session-out to accomplish this; there might be a few wrinkles to figure out but I hope to have a PR ready in the next day or two.

@brandonbloom

This comment has been minimized.

Copy link
Contributor

brandonbloom commented Jan 10, 2019

Please re-read my comment here:
brandonbloom/fipp#55 (comment)

I'm not proposing adding streaming value output to nREPL. I'm not proposing changing the RPC model in any way. I'm proposing adding a maximum string length to the returned payload and enforcing that limit on any/all printers. The implementation is backwards compatible for string-returning print functions and is pretty straightforward. This would solve the DoS problem, which has affected me many times.

Replace (printer result) with this:

(with-out-head n
  (print (str (printer result)))

Where with-out-head is something like:

(defmacro with-out-head [n & body]
  `(binding [*out* (TruncatingBuffer. n)]
     (try ~@body
       (catch BufferFullError e#))
     (.string *out*)))

And TruncatedBuffer is an appropriate implementation of java.io.Writer that throws BufferedFullError after n bytes have been written.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Jan 10, 2019

I understood your suggestion, but this is essentially the same as setting *print-length* (as to my knowledge all print functions respect this or at least are supposed to respect it), so I don't see what we'd gain by making this some eval request param. The streaming approach would provide better UX, even if it'll kill clients who don't interrupt the printing process quickly. At least people would quickly see what went wrong.

@brandonbloom

This comment has been minimized.

Copy link
Contributor

brandonbloom commented Jan 10, 2019

Setting *print-length* is not sufficient, not even in conjunction with *print-depth*. Consider the following with a limit of 100:

(repeat 100 (repeat 100 (range 100)))

Whoops! 1 million items printed.

Or even just:

(apply str (repeat 101 \x))

I've run in to excessive printing and OOM from nREPL in both classes of problems. When this occurs, I tend to fallback to direct evaluation in a streaming repl so that at least interrupts work correctly.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Jan 11, 2019

Ah, I keep forgetting about multi-dimensional data structures. Yeah, for this kind of problems your proposed solution certain makes sense, although if we switch to a streaming model it would only serve to protect the clients as the server itself it will become "immune" to the OOM problem.

Still, it's not a big change, so it's probably worth adding this as something optional.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Feb 23, 2019

Fixed in #118.

@bbatsov bbatsov closed this Feb 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.