tools.nrepl

Laziness-forcing loses *out* bindings

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: 0.2.2
  • Fix Version/s: 0.2.4
  • Component/s: None
  • Labels:
    None

Description

This was reported in https://github.com/trptcolin/reply/issues/129

My current theory, outlined there, is that nREPL sets up session bindings for `out`, which since we're creating a lazy seq are lost before nREPL goes to print out the seq for transport back to the nREPL client (REPLy, vim-fireplace, etc.).

user=> (defn foo [] (println "a") (map (fn [x] (println "b") (throw (Exception. "oops"))) [1 2 3]))
#'user/foo
user=> (foo)
a

Exception oops  user/foo/fn--699 (NO_SOURCE_FILE:1)
user=> (doall (foo))
a
b

Exception oops  user/foo/fn--699 (NO_SOURCE_FILE:1)

Don't have time at the moment to dig in, but can do so later if nobody else gets to it first.

  1. NREPL-45.diff
    25/Feb/14 9:36 AM
    3 kB
    Chas Emerick
  2. nrepl-45.patch
    21/Feb/14 5:19 PM
    4 kB
    Colin Jones

Activity

Hide
Chas Emerick added a comment -

Nope, thank you. I'll get a release out sometime this week.

Show
Chas Emerick added a comment - Nope, thank you. I'll get a release out sometime this week.
Hide
Colin Jones added a comment -

Yeah, I definitely like it better in pr-values, where the rest of the printing logic already lives. Committed as bbc5e02b665356e7dc33fe1d78346f17b3dec452.

Thanks!

Show
Colin Jones added a comment - Yeah, I definitely like it better in pr-values, where the rest of the printing logic already lives. Committed as bbc5e02b665356e7dc33fe1d78346f17b3dec452. Thanks!
Hide
Chas Emerick added a comment -

Take a look at this, Colin. Your solution was correct, but can just drop into pr-values; with-out-str was too crude a hammer for what pr-values needs.

If you can, verify that this makes REPL-y (and any other tools you're interested in) work as expected. Feel free to commit it, or I can (you should have privs IIRC).

Show
Chas Emerick added a comment - Take a look at this, Colin. Your solution was correct, but can just drop into pr-values; with-out-str was too crude a hammer for what pr-values needs. If you can, verify that this makes REPL-y (and any other tools you're interested in) work as expected. Feel free to commit it, or I can (you should have privs IIRC).
Hide
Chas Emerick added a comment -

I'm going to reply on the reply ticket :-P, as I think the issue probably isn't nREPL.

As for the patch itself, I'm suspicious of printing values twice as a fix for anything. If output isn't being captured properly, then we should find the problem with how we're capturing output, not throw more output at the problem.

Show
Chas Emerick added a comment - I'm going to reply on the reply ticket :-P, as I think the issue probably isn't nREPL. As for the patch itself, I'm suspicious of printing values twice as a fix for anything. If output isn't being captured properly, then we should find the problem with how we're capturing output, not throw more output at the problem.
Hide
Colin Jones added a comment -

OK, more data: this is happening because of https://github.com/clojure/tools.nrepl/blob/master/src/main/clojure/clojure/tools/nrepl/middleware/pr_values.clj#L23, where the (with-out-str (pr v)) could be the first time a lazy sequence gets forced, so any side effects' printing gets thrown away.

The trick is distinguishing the actual side-effect printing from the printable value when the sequence is being forced. It seems like it comes down to this: we need to force lazy sequences in the same way that printing does, while keeping the value and the print output distinct. The attached patch does this (I opted to make a local instead of using the private #'clojure.core/pr-on).

I think this is good, but wouldn't mind a review before I push it.

Show
Colin Jones added a comment - OK, more data: this is happening because of https://github.com/clojure/tools.nrepl/blob/master/src/main/clojure/clojure/tools/nrepl/middleware/pr_values.clj#L23, where the (with-out-str (pr v)) could be the first time a lazy sequence gets forced, so any side effects' printing gets thrown away. The trick is distinguishing the actual side-effect printing from the printable value when the sequence is being forced. It seems like it comes down to this: we need to force lazy sequences in the same way that printing does, while keeping the value and the print output distinct. The attached patch does this (I opted to make a local instead of using the private #'clojure.core/pr-on). I think this is good, but wouldn't mind a review before I push it.

People

Vote (1)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: