<< Back to previous view

[NREPL-17] Agent nesting causes issues Created: 13/Apr/12  Updated: 16/Apr/12  Resolved: 16/Apr/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.0

Type: Defect Priority: Blocker
Reporter: Colin Jones Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

We talked about this briefly on IRC/github: https://github.com/overtone/overtone/issues/85. I guess the nicest way to fix it would be to stop using agents in nREPL. There's only one spot where agents are created, and a few send-off calls (no sends), all internal, so any solution will be non-intrusive from an API perspective.

One implementation idea is to use a simple blocking queue and thread pool in java-land.

Another is to use nearly the same implementation as Agent, but remove the nesting restriction (call it NestableAgent?). Also get rid of the STM interaction and validator business since those aren't available outside the clojure.lang package - and then provide a parallel version of send-off. I've spiked a bit on this one, but I'm starting to think a plain queue/thread pool is cleaner.

Thoughts?



 Comments   
Comment by Chas Emerick [ 13/Apr/12 5:18 AM ]

Yes, I started working on this yesterday. Agents have definitely proven themselves unsuitable here, and a poor choice.

I'm not going to be fancy; we don't need fancy here.

Comment by Colin Jones [ 13/Apr/12 5:49 AM ]

Great news! Thanks for the quick response.

Comment by Chas Emerick [ 16/Apr/12 3:08 PM ]

Fixed in 0.2.0-beta6.





[NREPL-25] Metadata -> middleware/handler documentation Created: 10/Aug/12  Updated: 13/Aug/12  Resolved: 13/Aug/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.0-beta9

Type: Enhancement Priority: Blocker
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Comments   
Comment by Chas Emerick [ 13/Aug/12 11:31 PM ]

See clojure.tools.nrepl.middleware/wrap-describe, set-descriptor!, etc.





[NREPL-32] Handling of messages is serialized on a per-connection basis Created: 03/Oct/12  Updated: 04/Oct/12  Resolved: 04/Oct/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.0-beta10

Type: Defect Priority: Blocker
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Comments   
Comment by Chas Emerick [ 04/Oct/12 12:31 AM ]

Fixed @ 2f12262834e5b93b8536c81b12df25b2b75e0254.





[NREPL-26] Middleware metadata + collection of middleware names => well-formed handler Created: 10/Aug/12  Updated: 20/Aug/12  Resolved: 20/Aug/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.0-beta9

Type: Enhancement Priority: Blocker
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Comments   
Comment by Chas Emerick [ 20/Aug/12 3:48 PM ]

See clojure.tools.nrepl.middleware, the metadata descriptors described there, etc.





[NREPL-49] Chas, where are you? You said you were going to the Hops & Hominy restaurant Created: 25/Mar/14  Updated: 28/Mar/14  Resolved: 28/Mar/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Blocker
Reporter: Andy Fingerhut Assignee: Chas Emerick
Resolution: Not Reproducible Votes: 0
Labels: bug
Environment:

San Francisco


Attachments: JPEG File IMG_20140326_174912.jpg    

 Description   

You said you, Gary, and Chris were going to meet me at Hops & Hominy restaurant after Clojure/West. I am here, but I don't see you guys anywhere.

Steps to reproduce: Walk around the restaurant, looking for you at every table. You are not there.



 Comments   
Comment by Chas Emerick [ 26/Mar/14 2:13 PM ]

This is officially the most awesome JIRA ticket, ever. <3 Andy. :-D

Comment by Chas Emerick [ 26/Mar/14 2:45 PM ]

People keep asking, so: yes, we got there and all had a good time.

Comment by Justin Balthrop [ 26/Mar/14 7:40 PM ]

Chas, I'm having a similar issue. You said you'd be around, but I can't find you.

Comment by Aaron Brooks [ 26/Mar/14 7:55 PM ]

I've re-run the tests and found Chas to be in the Garden Court of the Palace Hotel. See the attached test results.

Comment by Chas Emerick [ 28/Mar/14 12:07 PM ]

@Justin, this is likely a race condition. I was there (as indicated by the test results, thanks Aaron), but you weren't. If Reid had finished his test.check scheduler, we could have prevented a buggy interleaving. Since we've all decamped from SF at this point, I don't know that we'll be able to properly reach a resolution.

I'm closing this as 'not reproducible' for now.





[NREPL-12] Intermittent test failures for stdin Created: 26/Feb/12  Updated: 10/Jun/12  Resolved: 10/Jun/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.0

Type: Defect Priority: Critical
Reporter: Colin Jones Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Improve-timing-issues-in-tests.patch    

 Description   

Failures like these are happening sometimes in the test suite, mainly around stdin:

FAIL in (request-multiple-read-objects-in) (nrepl_test.clj:211)
expected: (= (quote (:ohai)) (response-values (for [resp (repl-eval session "(read)")] (do (when (-> resp :status set (contains? "need-input")) (session {:op :stdin, :stdin ":ohai :kthxbai\n"})) resp))))
actual: (not (= (:ohai) nil))

FAIL in (request-multiple-read-objects-in) (nrepl_test.clj:217)
expected: (= [" :kthxbai"] (repl-values session "(read-line)"))
actual: (not (= [" :kthxbai"] nil))

I've done a ton of debugging and thinking about this, and haven't gotten far. A few theories I had that seemed decent at the time, but ultimately haven't gotten rid of these occasional failures:

The debugging I've been doing (print statements galore) suggests that when this problem happens, the client does send back the proper :stdin, but it doesn't get picked up by the add-stdin handler in time to write to the :stdin-writer and avoid the expiring timeout.

I'm pretty much stumped at this point, unfortunately.



 Comments   
Comment by Chas Emerick [ 29/Mar/12 4:18 PM ]

This is becoming a serious problem; the build fails half the time on build.clojure.org, making it incredibly frustrating to perform releases, etc.

Any new ideas / evaluation of this is most welcome.

Comment by Colin Jones [ 15/Apr/12 10:24 PM ]

My current vague suspicion is that it's a CPU contention thing between the server and client.

It's kind of hard for me to reason about the execution of the response-values lazy seq, and where the blocking actually happens. And of course adding logging changes the behavior sometimes. I wouldn't be opposed to adding sleeps in there, but I expect those kinds of things would still be flaky later, on build machines in particular :/

Incidentally, bumping the client timeout up to Long.MAX_VALUE helps a bit the stdin failures, but doesn't solve them. It does create new ones: session-lifecycle, unknown-op, and read-timeout block forever trying to reduce across the whole seq in combine-responses. It was a bit surprising to me to see that reduce in the response-values pipeline, and I pursued that at one point as well, but it hasn't gotten me anywhere. Decreasing the timeout helps to reproduce the failures.

I got this thread dump from a blocking-forever stdin test (run via `lein test clojure.tools.nrepl-test`): https://refheap.com/paste/2141.

Comment by Colin Jones [ 03/Jun/12 5:07 AM ]

With this patch, I was able to run clojure.tools.nrepl-test 100 times successfully. Previous attempts would generally only get as far as 15 or so before failing for one reason or another.

It also gets rid of a race condition with my skip-if-eol stuff that was addressing multiple input reads. Lastly, it avoids the need for the 10-second timeout, so the tests are faster now as well.

Comment by Chas Emerick [ 09/Jun/12 10:57 AM ]

I'm psyched that you may have found the solution to these problems. I've applied the patch locally, and it tests well. I want to eyeball it for a minute (probably tomorrow) before moving on.

Comment by Chas Emerick [ 10/Jun/12 4:40 PM ]

Pushed, will be in the next beta release. Thanks so much!





[NREPL-31] REPL utilities are refered into *ns* prior to every expression evaluation Created: 01/Oct/12  Updated: 22/Oct/12  Resolved: 22/Oct/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.0-beta9
Fix Version/s: 0.2.0, 0.2.0-RC1

Type: Defect Priority: Critical
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Declined Votes: 0
Labels: None


 Description   

The refers of REPL utilities (pst, pprint, etc) happens in clojure.main/repl. In a terminal/single-threaded REPL, this is called just once, so it only ever affects the user namespace. Every expression sent for evaluation by nREPL invokes clojure.main/repl though, so moving *ns* around will inadvertently cause those refers to happen over and over, into non-user namespaces. (I've been enjoying having pprint and pp available all the time, but I'd never thought much about why they were always there.)

In the end, this is an nREPL bug.

I don't see any easy way out off the top of my head. I think nREPL will end up having to stop using clojure.main/repl, and maintain a modified version of it itself (something I wanted to avoid exactly so as to benefit from the changes to clojure.main/repl from version to version of Clojure).

Suggestions most welcome.

(Originally reported here.)



 Comments   
Comment by Chas Emerick [ 11/Oct/12 2:10 PM ]

Hopefully CLJ-1085 is resolved and this can drop off. In the meantime, not going to hold off -beta10 for it or this.

Comment by Chas Emerick [ 22/Oct/12 8:01 PM ]

Fixed upstream in CLJ-1085. Note that this issue will continue to affect those using Clojure < 1.5.0.





[NREPL-2] send-ack probably leaks connections/sockets Created: 30/Sep/11  Updated: 21/Feb/12  Resolved: 21/Feb/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.0

Type: Defect Priority: Critical
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

The let in send-ack should be be with-open.






[NREPL-53] Middleware linearization is nondeterministically wrong Created: 02/May/14  Updated: 19/Jan/15  Resolved: 12/Jan/15

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.3
Fix Version/s: 0.2.7

Type: Defect Priority: Critical
Reporter: Gary Fredericks Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None
Environment:

clojure 1.6.0


Attachments: Text File NREPL-53.patch    

 Description   

I've had trouble getting my middleware ordered correctly, and have wittled it down to the following example which seems to only fail in some processes. I.e., it is deterministic within a particular jvm, but across jvms the result can change. My guess is that this has to do with hashing vars.

This code prints true (signifying a correct result) roughly 1/3 of the time and false otherwise.

(ns chas.core
  (:require [clojure.tools.nrepl.middleware :as mid]
            [clojure.tools.nrepl.middleware.pr-values]
            [clojure.tools.nrepl.middleware.session]))

(def wrap-foo identity)
(mid/set-descriptor! #'wrap-foo
                     {:expects #{#'clojure.tools.nrepl.middleware.session/session "eval"}})

(def wrap-bar identity)
(mid/set-descriptor! #'wrap-bar
                     {:requires #{#'clojure.tools.nrepl.middleware.session/session},
                      :expects #{"describe" #'clojure.tools.nrepl.middleware.pr-values/pr-values},
                      :handles {"stacktrace" {}}})

(defn -main
  []
  (println
   ;; should return true, since #'wrap-foo belongs later in the stack than
   ;; #'clojure.tools.nrepl.middleware.session/session due to its :expects
   ;; clause
   (->> [#'wrap-foo
         #'wrap-bar
         #'clojure.tools.nrepl.middleware.session/session]
        (mid/linearize-middleware-stack)
        (filter #{#'clojure.tools.nrepl.middleware.session/session #'wrap-foo})
        (= [#'clojure.tools.nrepl.middleware.session/session #'wrap-foo]))))

Leiningen makes it difficult to use forked versions of nrepl, so as a hackier workaround until this issue is resolved I've created this Leiningen plugin.



 Comments   
Comment by Gary Fredericks [ 02/May/14 10:35 PM ]

Interesting followup observation I made with Alan Malloy in IRC: if we remove "eval" from the :expects clause, it seems to return consistently true. On the other hand if we instead add #'clojure.tools.nrepl.middleware.interruptible-eval/interruptible-eval to the middleware list, suddenly it returns consistently false.

Comment by Gary Trakhman [ 04/Jun/14 3:35 PM ]

I should add my own experience report with this, hope to dig in to it someday soon: https://groups.google.com/d/msg/clojure/nUBBbYZHuTE/ScLBH-A2HkoJ

In my case, it was incorrectness, not indeterminism.

Comment by Chas Emerick [ 19/Aug/14 1:59 PM ]

Half of the middleware linearization stuff is too clever, the other half is really dumb. :-/

I'd love a patch for this. Absent one, I'll have to dig into it, but I'm not going to hold up 0.2.4 any further for that opportunity to come around.

Comment by Gary Fredericks [ 19/Aug/14 2:12 PM ]

Is this a fair summary of the requirements?

1) Should be deterministic
2) Should only return a result that respects the :expects and :requires clauses – if this is impossible it should throw an exception explaining why

Comment by Chas Emerick [ 19/Aug/14 2:58 PM ]

Yes, that's fair. I might add (3) try to not break anything significant that might rely upon undocumented behaviour, but I doubt that's helpful.

Comment by Gary Fredericks [ 25/Sep/14 9:29 PM ]

I've written generative tests for this, but test.check requires clojure 1.4 (while nrepl tests run on 1.2).

If I wrap them in code that NOOPs for old clojures, I imagine they will they get run in CI? Is that a reasonable approach?

Comment by Colin Jones [ 26/Sep/14 7:50 AM ]

That approach sounds great to me.

Comment by Gary Fredericks [ 26/Sep/14 10:06 AM ]

Given I've coded it that way, is there an easy way for me to run the tests without tweaking the pom.xml to specify 1.4?

Comment by Gary Fredericks [ 30/Sep/14 10:11 PM ]

Okay I think I finally tracked this down.

Basically the conj-sorted function was not a valid way of doing a topological sort.

I replaced it with an actual topological sort, that still pushes independent middlewares toward the end.

The generative tests are now passing, as are the rest of the tests.

At some point I'll circle back to my original problem and see if that's solved.

I still don't fully understand all of the linearization code, so it's possible that the problem is more subtle than this.

The fix is on a github branch here. I might want to wait on another release of test.check before making a patch, since it should have some new features that make the generative tests better (in particular I'd be able to remove the dependency on my test.chuck utility library, which obviously shouldn't be in the tools.nrepl codebase).

Comment by Gary Fredericks [ 04/Oct/14 10:07 AM ]

Attached a patch that contains the aforementioned topological sort, with a single regression test (and no generative tests).

Things to note:

  • I created a wrapper for clojure.core/ex-info so that in the case of a dependency cycle we can include with the exception message an example cycle, but don't have to assume clojure 1.4. If assuming clojure 1.4 is okay I can take that out and use clojure.core/ex-info directly.
  • I tried adding a test to verify that a dependency cycle creates an exception, but the test failed, and when I dug into it I discovered that the extend-deps function was doing strange things I did not expect, which means either it is buggy or I do not understand what it is supposed to do. See this commit for the failing test.
  • The topological sort now explicitly pushes middlewares with no dependencies to the end of the stack, instead of the more implicit fashion in the older code.
  • This patch does fix the problem I described in the description
Comment by Chas Emerick [ 12/Jan/15 8:23 AM ]

Great, thanks for the work! Did you have specification tests written, and then stripped them out? If so, I'd love to include them.

Comment by Gary Fredericks [ 12/Jan/15 10:05 AM ]

There are some generative tests somewhere that I think Colin was helping with – is that what you're referring to?

Comment by Chas Emerick [ 12/Jan/15 10:15 AM ]

No, you mentioned your test.chuck library, and waiting for a new test.check release before making a patch...it sounded like you had spec tests written, but then removed them for the patch.

Comment by Colin Jones [ 12/Jan/15 10:21 AM ]

Yeah I was trying to help with those, but got kind of hung up on what happens when there's a dependency cycle (a stack overflow, IIRC, instead of the `IllegalArgumentException` with a nice error message that I'd have wanted). Also sidetracked by other priorities - sorry Gary!

But I think that's a separate problem with `clojure.tools.nrepl.middleware/dependencies` not tracking nodes it has already visited: not caused by Gary's patch.

Comment by Chas Emerick [ 12/Jan/15 10:32 AM ]

OK, thanks for the clarification. I'll merge this now, but wait a few days to release to see if anything breaks from people using the snapshot. (Perhaps a good/expected thing given the shortcomings of conj-sorted.)

Comment by Gary Fredericks [ 12/Jan/15 11:10 AM ]

Oh right, the issue was that the generators for the tests were written using this helper macro that I've become quite attached to, and so far I haven't been able to convince Reid that it should be in test.check proper.

The tools.nrepl tests are here, and you can imagine how readable this would be without the for macro.

Comment by Chas Emerick [ 12/Jan/15 11:19 AM ]

Applied to master @ 8547857, now available in 0.2.7-SNAPSHOT in the OSS snapshots repo: https://oss.sonatype.org/content/repositories/snapshots

Comment by Gary Fredericks [ 12/Jan/15 11:32 AM ]

Thanks Chas!

Comment by Gary Fredericks [ 18/Jan/15 4:28 PM ]

Wait is there actually a problem with using a non-contrib library if it's just for tests?

Comment by Chas Emerick [ 19/Jan/15 4:04 AM ]

I can't see why that'd be a problem. Let's have the test-scoped dependency be a non-SNAPSHOT though.

Comment by Chas Emerick [ 19/Jan/15 4:08 AM ]

That said, if you can write the tests you want without using test.chuck, that'd be even better, just so that later maintainers don't have to grok those helpers. I see the utility of them (especially for), but it could still be a barrier compared to vanilla simple-check bits.

Comment by Gary Fredericks [ 19/Jan/15 7:56 AM ]

Okay, I've got a refactoring of the tests without test.chuck, I just have a bit of prep to do and then I'll make another ticket/patch.





[NREPL-19] Android: nREPL starts with no namespace Created: 11/May/12  Updated: 20/Jun/12  Resolved: 20/Jun/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.0
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Alexander Yakushev Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: android,, bug
Environment:

Android w/ Clojure 1.4.0, REPLy client / Eclipse CCW client



 Description   

When I start the nREPL on the Android device (by calling `(clojure.tools.nrepl.server/start-server :port 9999)`) it all goes well. But when I try to connect to this REPL using any of the clients I find myself in an empty namespace (or something like that). The var ns is unbound, no functions from the clojure.core are available. At the beginning REPLy tries to perform some actions but they fail (says that it cannot find symbol `defn` - because nothing from clojure.core is being mapped).

However I can do (in-ns 'anywhere) it works. Everything else in the REPL works correctly (as far as I see). The issue itself is minor but I'm afraid that it is caused by some crash during nREPL initialization that might lead to other problems in future.



 Comments   
Comment by Alexander Yakushev [ 11/May/12 11:06 AM ]

Can't edit, I meant the *ns* var, of course.

Comment by Alexander Yakushev [ 19/May/12 4:32 PM ]

With the help of Daniel Solano Gómez I managed to fix this bug. The problem was caused by the lack of user namespace in the Android-patched Clojure. nREPL assumes that the user namespace is present and uses it by default.

Here's the so called fix I ended up with:

...
(let [user-ns (create-ns 'user)]
  (binding [*ns* user-ns]
    (clojure.tools.nrepl.server/start-server :port 9999)))
...

The issue can be closed now.

Comment by Chas Emerick [ 27/May/12 4:04 PM ]

A follow up Q: user is created by clojure.lang.RT's static initialization. Is the lack of that in "Android-patched Clojure" an optimization of some sort?

Comment by Alexander Yakushev [ 27/May/12 4:49 PM ]

Exactly, Daniel Solano Gómez removed it because it's initialization took additional time, it seems.





[NREPL-23] NPE when interruptible-eval/evaluate is passed a non-existant namespace Created: 22/Jul/12  Updated: 20/Aug/12  Resolved: 20/Aug/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.0
Fix Version/s: 0.2.0-beta9

Type: Defect Priority: Major
Reporter: Joe Snikeris Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   
Stack Trace
Exception in thread "nREPL-worker-2" java.lang.NullPointerException
	at clojure.core$refer.doInvoke(core.clj:3779)
	at clojure.lang.RestFn.applyTo(RestFn.java:139)
	at clojure.core$apply.invoke(core.clj:603)
	at clojure.core$load_lib.doInvoke(core.clj:5279)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:603)
	at clojure.core$load_libs.doInvoke(core.clj:5298)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:605)
	at clojure.core$use.doInvoke(core.clj:5392)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.main$repl.doInvoke(main.clj:259)
	at clojure.lang.RestFn.invoke(RestFn.java:1096)
	at clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__337.invoke(interruptible_eval.clj:51)
	at clojure.lang.AFn.applyToHelper(AFn.java:159)
	at clojure.lang.AFn.applyTo(AFn.java:151)
	at clojure.core$apply.invoke(core.clj:601)
	at clojure.core$with_bindings_STAR_.doInvoke(core.clj:1771)
	at clojure.lang.RestFn.invoke(RestFn.java:425)
	at clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invoke(interruptible_eval.clj:36)
	at clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn__374$fn__376.invoke(interruptible_eval.clj:162)
	at clojure.core$comp$fn__4034.invoke(core.clj:2278)
	at clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__367.invoke(interruptible_eval.clj:129)
	at clojure.lang.AFn.run(AFn.java:24)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1110)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:603)
	at java.lang.Thread.run(Thread.java:636)


 Comments   
Comment by Joe Snikeris [ 22/Jul/12 11:23 AM ]

I'm not sure which would be better: behave as if one wasn't passed in or return an error on the transport.

Comment by Chas Emerick [ 20/Aug/12 9:40 PM ]

Fixed to send an error with a status of "namespace-not-found".





[NREPL-11] Ensure efficient bytestring (byte[]) support in bencode transport Created: 21/Feb/12  Updated: 21/Aug/12  Resolved: 21/Aug/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.0-beta9

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

In particular, no boxing of individual bytes or multiple en/decoding steps should be involved.

This implies that:

1. all string values should remain byte[] coming out of the bencode protocol support (which makes it more compliant with bencode anyway)
2. the string decoding should occur in the bencode transport impl
3. all string values should be decoded, except for those named in a reserved slot (say, "-unencoded")

All map keys should always be decoded to Strings.

While nREPL should continue to be fully compatible with Clojure 1.2.x, let's not kill ourselves trying to optimize for 1.2.x as well as Clojure >= 1.3.0; if necessary, just make sure the latter is fast. If 1.2.x benefits as well, then good.



 Comments   
Comment by Meikel Brandmeyer [ 22/Feb/12 2:05 PM ]

How about “nrepl/unencoded” re point 3?

Comment by Chas Emerick [ 22/Feb/12 5:57 PM ]

Is that intended to become a namespaced keyword at some point (further up in the stack than the bencode parser or transport, of course)?

If so, does that imply that other keys (perhaps "nonstandard" ones defined by middlewares) should be namespaced as well?

Also, this is actually impacting the protocol; nrepl/* makes me think the slot is related to an "application-level" handler provided by nREPL.

FWIW, my suggestion of -unencoded was a nod to Python's _foo and Clojure's -foo to denote private / internal members. *shrug*

Comment by Meikel Brandmeyer [ 23/Feb/12 5:03 AM ]

Namespaced keywords are indeed the inspiration. So that it should probably be more like clojure.tools.nrepl.transport/unencoded rather than just nrepl/unencoded. However it is not a requirement to be a real keyword. A qualified string works just as well. There could be a middleware which turns the map keys into keywords (considering also a possible namespace). ring has a similar middleware, IIRC. I think that qualified operation and result field values (in some form or the other) are a best practise in this respect.

However your argument of unencoded being part of the protocol is valid. But then why should it be private? Document it in the official protocol spec and let middlewares also add fields to it. The question is then: what happens with nested structures (eg. :status, which has to be traversed now painfully)? If middlewares weren't allowed to add to it, then only :value would be an interesting target for unencoded. So a simple flag would suffice.

In general, I still think a tagged protocol would have been the better solution since it also allows nesting and each slot knows whether it's encoded or not.

Maybe we should clarify what middlewares are allowed to do. Is there already some documentation on this? (I'm must confess I'm not up-to-date with all the design changes.)

Comment by Chas Emerick [ 01/Mar/12 3:32 PM ]

Re: encoding, I've started to think perhaps nothing should be decoded by default, and middlewares can independently handle such things. The standard middlewares will stake out one approach, but that at least leaves open the possibility of a different convention emerging over time that can be adopted later.

Opinions?

Comment by Chas Emerick [ 20/Aug/12 4:14 PM ]

Sorry, where is the code for this? No .patch here, and nothing in bitbucket looks right.

Leaving aside the code for now, defaulting to unencoded strings continues to seem reasonable; a default Transport impl/substrate can perform the en/decoding, as well as establish a default convention for indicating that some fields values should be left unencoded and should not be decoded.

Comment by Meikel Brandmeyer [ 20/Aug/12 4:21 PM ]

The code is here: https://github.com/kotarak/tools.nrepl/tree/bencode-improvements2

Comment by Chas Emerick [ 20/Aug/12 10:14 PM ]

Looks good, patch applied.

I'm less and less sure re: -unencoded. Thinking back to your "tagged protocol" comment, adopting a convention of keys like "value/binary" (or something similar) would be quite a bit more flexible and somewhat simpler to handle.

(Thinking of rich content, mime types could then be sent in e.g. "value/mime" — not that the transport would care about that, but it would at least be easy to pair up data with its "tag".)

Thoughts?

Comment by Chas Emerick [ 21/Aug/12 8:26 PM ]

Releasing -beta9 now; further discussion of unencoded value semantics can continue elsewhere…





[NREPL-20] Stray "#" in session-out is breaking (.println *out*) Created: 09/Jun/12  Updated: 09/Jun/12  Resolved: 09/Jun/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.0
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Alex Osborne Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Patch: Code and Test
Waiting On: Chas Emerick

 Description   

There's a stray "#" in clojure.tools.nrepl.middleware.session/session-out which is turning the body of the (not off) case into a lambda that's never executed:

(cond
(number? x) (.append buf (char x))
(not off) #(.append buf x) ; <----
(instance? CharSequence x) (.append buf ^CharSequence x off len)
:else (.append buf ^chars x off len))

https://github.com/clojure/tools.nrepl/blob/master/src/main/clojure/clojure/tools/nrepl/middleware/session.clj#L34

This is causing the newline character in (.println out) and (.println err) to be lost:

user=> (with-open [conn (repl/connect :port 7888)]
(-> (repl/client conn 1000)
(repl/message {:op :eval :code "(do (.println out \"1\") (.println out \"2\") (.println out \"3\")(flush))"})
doall
pprint))
({:out "123", ;; <--- where are my "\n"s?
...}, ...)

Removing the stray "#" fixes the problem:

{:out "1\n2\n3\n",
...}

Reported downstream by jaceklaskowski in https://github.com/technomancy/leiningen/issues/631

Patch with the trivial fix and unit test here: https://wok.meshy.org/~ato/0001-Remove-stray-hash-in-session-out-that-breaks-println-out.patch

Couldn't attach the patch to this issue as JIRA told me I don't have permission. I (Alex Osborne) have signed the Clojure CA and contribute this patch as per the terms of it.



 Comments   
Comment by Colin Jones [ 09/Jun/12 9:44 AM ]

Looks good, nice catch. Verified this test fails before the fix.

Chas, want me to apply/push this? I'm happy to do that, but wanted to wait since we haven't really talked about project workflow.

Comment by Chas Emerick [ 09/Jun/12 10:54 AM ]

Thank you very much — patch applied to master. I'll cut a new beta release on Monday so downstreams can pick it up.

Good eye, by the way, I'm sure I could have looked for days for that.





[NREPL-13] nrepl should support binding to a specific interface instead of all Created: 29/Feb/12  Updated: 29/Mar/12  Resolved: 29/Mar/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.0

Type: Enhancement Priority: Major
Reporter: Toby Crawley Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Patch: Code

 Description   

nrepl currently accepts a :port to bind to as an option, and uses that with the single-arg ServerSocket constructor, which binds to that port on all interfaces. I propose we add a :host option as well that can be used to specify the hostname or address of the interface to bind to.

Here is a potential implementation:

diff --git a/src/main/clojure/clojure/tools/nrepl/server.clj b/src/main/clojure/clojure/tools/nrepl/server.clj
index 9dbac40..b6f625b 100644
--- a/src/main/clojure/clojure/tools/nrepl/server.clj
+++ b/src/main/clojure/clojure/tools/nrepl/server.clj
@@ -8,7 +8,7 @@
                                             pr-values
                                             session))
   (:use [clojure.tools.nrepl.misc :only (returning response-for log)])
-  (:import (java.net Socket ServerSocket)))
+  (:import (java.net InetAddress Socket ServerSocket)))
 
 (defn unknown-op
   "Sends an :unknown-op :error for the given message."
@@ -64,7 +64,9 @@
 
 (defn start-server
   "Starts a socket-based nREPL server.  Configuration options include:
- 
+
+   * :host — the address or hostname of the interface that should be used;
+       defaults to all interfaces
    * :port — defaults to 0, which autoselects an open port on localhost
    * :handler — the nREPL message handler to use for each incoming connection;
        defaults to the result of (default-handler)
@@ -77,8 +79,10 @@
 
    Returns a handle to the server that is started, which may be stopped
    either via `stop-server`, (.close server), or automatically via `with-open`."
-  [& {:keys [port transport-fn handler ack-port greeting-fn] :or {port 0}}]
-  (let [ss (ServerSocket. port)
+  [& {:keys [host port transport-fn handler ack-port greeting-fn] :or {port 0}}]
+  (let [ss  (if host
+              (ServerSocket. port 0 (InetAddress/getByName host))
+              (ServerSocket. port))
         smap {:ss ss
               :transport (or transport-fn t/bencode)
               :greeting greeting-fn

I'm putting a CA in the mail today.



 Comments   
Comment by Chas Emerick [ 29/Mar/12 12:53 PM ]

Fixed on master, released as part of 0.2.0-beta3.





[NREPL-14] Exception when running -main from clojure.tools.nrepl.cmdline Created: 06/Mar/12  Updated: 20/Jun/12  Resolved: 20/Jun/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.0
Fix Version/s: 0.2.0

Type: Defect Priority: Major
Reporter: Trent Ogren Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Fix-calls-using-old-function-signatures.patch    
Patch: Code

 Description   

Running -main from clojure.tools.nrepl.cmdline gives the following exception:

Exception in thread "main" java.lang.IllegalArgumentException: No value supplied for key: 0
at clojure.lang.PersistentHashMap.createWithCheck(PersistentHashMap.java:89)
at clojure.core$hash_map.doInvoke(core.clj:355)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:600)
at clojure.tools.nrepl.server$start_server.doInvoke(server.clj:65)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.tools.nrepl.cmdline$_main.doInvoke(cmdline.clj:74)
at clojure.lang.RestFn.invoke(RestFn.java:397)
at clojure.lang.Var.invoke(Var.java:397)
at clojure.lang.AFn.applyToHelper(AFn.java:159)
at clojure.lang.Var.applyTo(Var.java:518)
at clojure.core$apply.invoke(core.clj:600)
at clojure.main$main_opt.invoke(main.clj:323)
at clojure.main$main.doInvoke(main.clj:426)
at clojure.lang.RestFn.invoke(RestFn.java:421)
at clojure.lang.Var.invoke(Var.java:405)
at clojure.lang.AFn.applyToHelper(AFn.java:163)
at clojure.lang.Var.applyTo(Var.java:518)
at clojure.main.main(main.java:37)

It looks like the repl/connect and start-server functions have changed since cmdline.clj was written. I attached a patch that updates their usage.



 Comments   
Comment by Chas Emerick [ 20/Jun/12 7:36 AM ]

Merged, thanks!





[NREPL-38] Certain Calendar values don't seem to be able to print Created: 12/Jan/13  Updated: 13/Jan/13  Resolved: 13/Jan/13

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.0
Fix Version/s: 0.2.1

Type: Defect Priority: Major
Reporter: Julian Birch Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: bug
Environment:

Ubuntu 12.10, running through Leiningen 2



 Description   

If I try to run lein repl outside of a project folder, I get Clojure 1.4.0.
I then run

(println (javax.xml.bind.DatatypeConverter/parseDateTime "2008-07-21T19:17:29"))

which produces

IndexOutOfBoundsException start 26, end 2, s.length() 28 java.lang.AbstractStringBuilder.append (AbstractStringBuilder.java:476)
#inst "2008-07-21T19:17:29.000+01:

Note the absence of a closing '"'.

My apologies in advance if this is impossible to reproduce.



 Comments   
Comment by Andy Fingerhut [ 13/Jan/13 9:55 AM ]

I can reproduce this with Clojure 1.4.0 and 1.5.0-RC2 if I do it within "lein2 repl" (I was using Leiningen version 2.0.0-preview10 to reproduce the problem).

If I use "java -cp clojure.jar clojure.main" to start a REPL session, with either Clojure 1.4.0 or 1.5.0-RC2 for clojure.jar, I don't see any problem. I was testing on Mac OS X 10.6.8 with Oracle/Apple JDK 1.6.0_37.

I also don't see this problem if I use Leiningen version 1.7.1, tested with both Clojure 1.4.0 and 1.5.0-RC2.

This appears to be some kind of bad interaction between Leiningen 2.0.0-preview10 and Clojure.

Comment by Andy Fingerhut [ 13/Jan/13 10:05 AM ]

I also reproduced this issue with the latest version of Leiningen, which is 2.0.0-RC2. Email sent to the Leiningen developer email list so they know about it.

Comment by Chas Emerick [ 13/Jan/13 10:52 AM ]

This is an nREPL bug involving an API mismatch between java.io.Writer.write() and java.lang.AbstractStringBuilder.append().

The fix is simple; patch release coming later today.

Comment by Chas Emerick [ 13/Jan/13 12:29 PM ]

Fixed with b9e930a1.

Will be a part of [org.clojure/tools.nrepl "0.2.1"], to be released later today.





[NREPL-16] nrepl.middleware.interruptible_eval/interruptible_eval raises a stack inconsistence Exception if the call to clojure.main/repl fails Created: 12/Apr/12  Updated: 20/Apr/12  Resolved: 16/Apr/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.0
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Alexander Yakushev Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: bug
Environment:

clojure 1.4.0-beta5, Android



 Description   

If the call to clojure.main/repl in `evaluate' raises an exception before the :init part gets evaluated (so the expression `(push-thread-bindings @bindings)' is executed) then nREPL crashes with the following exception:

java.lang.IllegalStateException: Pop without matching push

It happens because after the underlining exception in clojure.main/repl is caught by the try block in `evaluate', the `finally' black calls (pop-thread-bindings) which were not actually "pushed".



 Comments   
Comment by Chas Emerick [ 16/Apr/12 5:34 PM ]

Fixed in 0.2.0-beta6. Please give it a try and see how it works on Android.

Comment by Alexander Yakushev [ 20/Apr/12 1:40 PM ]

It is OK now. OK in a sense that if something wrong happens inside the clojure.main/repl function then the stacktrace points there after the application dies.
Thanks for you help!





[NREPL-30] Investigate hangs when server goes away Created: 14/Sep/12  Updated: 08/Oct/12  Resolved: 08/Oct/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.0-beta9, 0.2.0-beta10
Fix Version/s: 0.2.0-beta10

Type: Task Priority: Major
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

See https://github.com/trptcolin/reply/issues/84#issuecomment-8481753 and maybe http://code.google.com/p/counterclockwise/issues/detail?id=405



 Comments   
Comment by Colin Jones [ 14/Sep/12 9:03 AM ]

If it helps, I did observe in some testing that after the server side of the socket goes down, writing to and flushing the client socket's output stream twice would bubble up a client exception. That seems a little keepalive-ish, but I'm not sure whether that sort of thing would do other transports any good.

Comment by Chas Emerick [ 03/Oct/12 12:37 PM ]

A fix for this is on master now, with commit 8a5dad2045434fcc06f2878de55f7dcdefa01a1b. There were a number of issues in the implementation of FnTransport that were preventing exceptions from bubbling up as they should have been.

No APIs were harmed in the course of implementing a fix. Also, no keepalive mechanism was required (which makes me slightly suspicious that the issue has actually been fixed, but we'll see). (Not TCP keepalive; that's irrelevant to the issue, and wouldn't help even if it were reliably standard/present.)

New tests added, and manual verification against nREPL servers running in different processes looks good too. I'll put out an 0.2.0-beta10 shortly so that people can start banging on this to see if it resolves issues in downstream projects/tools.

Note that I also found that it sometimes requires two sends (messages here, not writes to a socket) to provoke an error when writing to a transport that had been connected to a server that was closed or otherwise went away. Maybe it's a buffering issue? I don't have a good theory. Check out e.g. nrepl-test/transports-fail-on-disconnects if you want to play around with it.

Comment by Chas Emerick [ 04/Oct/12 5:06 PM ]

Note that the bencode transport should throw a java.net.SocketException when its connection is lost.

Comment by Chas Emerick [ 08/Oct/12 12:42 PM ]

Good reports received from downstream projects on the fix for this. Calling it good.





[NREPL-1] Whitespace is not printed via :out Created: 30/Sep/11  Updated: 21/Feb/12  Resolved: 21/Feb/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.0

Type: Defect Priority: Major
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

This:

=> (println " ")
nil

Should be this:

=> (println " ")
 
nil

Perhaps :out isn't being sent if the content {{.trim}}s down to a zero-length string?



 Comments   
Comment by Chas Emerick [ 21/Feb/12 5:54 AM ]

Resolved in the redesign.





[NREPL-6] Add simple HOWTO to README for both tooling and application developers Created: 30/Sep/11  Updated: 21/Nov/12  Resolved: 21/Nov/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.0

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

...maybe push the protocol details into a /doc directory.






[NREPL-5] Add support for reading from stdin Created: 30/Sep/11  Updated: 21/Feb/12  Resolved: 21/Feb/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.0

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

Swank does this by polling back to the connected client to request input. In the face of asynchronous operations, this may be the only option.

Will require adding to the protocol.



 Comments   
Comment by Chas Emerick [ 21/Feb/12 5:58 AM ]

See clojure.tools.nrepl.middleware.session/add-stdin for the implementation. Userland documentation coming.





[NREPL-34] Default load-file middleware cannot load large files due to JVM method size limitations Created: 11/Oct/12  Updated: 11/Oct/12  Resolved: 11/Oct/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.0-beta10
Fix Version/s: 0.2.0-RC1

Type: Defect Priority: Major
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

Simply interpolating contents of files to be loaded into a Compiler/load expression to be evaluated does not work with very large files. (See http://code.google.com/p/counterclockwise/issues/detail?id=429)



 Comments   
Comment by Chas Emerick [ 11/Oct/12 11:34 PM ]

Fixed in https://github.com/clojure/tools.nrepl/commit/77cb7d2347b0a78094ea32d7757aa3e3662fc634





[NREPL-52] Middleware linearizes wrong with redundant :expects entries Created: 02/May/14  Updated: 02/May/14  Resolved: 02/May/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Gary Fredericks Assignee: Chas Emerick
Resolution: Duplicate Votes: 0
Labels: None


 Description   

Consider the following:

(def wrap-foo identity)
(mid/set-descriptor! #'wrap-foo
                     ;; note that these two entries are redundant since
                     ;; the session middleware implements "clone"
                     {:expects #{#'clojure.tools.nrepl.middleware.session/session "clone"}})

;; this one returns correctly
(mid/linearize-middleware-stack [#'wrap-foo
                                 #'clojure.tools.nrepl.middleware.session/session])
;; => (#'clojure.tools.nrepl.middleware.session/session
       #'user/wrap-foo)


;; this one returns incorrectly -- note that by adding a presumably
;; unrelated extra middleware to the stack, session and wrap-foo have
;; switched their order relative to each other.
(mid/linearize-middleware-stack [#'wrap-foo
                                 #'cider.nrepl.middleware.stacktrace/wrap-stacktrace
                                 #'clojure.tools.nrepl.middleware.session/session])
;; => (#'user/wrap-foo
       #'clojure.tools.nrepl.middleware.pr-values/pr-values
       #'cider.nrepl.middleware.stacktrace/wrap-stacktrace
       #'clojure.tools.nrepl.middleware.session/session)


 Comments   
Comment by Gary Fredericks [ 02/May/14 10:08 PM ]

I'm going to close this since I think NREPL-53 gets more to the root of the matter.





[NREPL-60] Tests failing in build "Unable to resolve var: clojure.pprint/use-method in this context" Created: 04/Aug/14  Updated: 19/Aug/14  Resolved: 19/Aug/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.4

Type: Defect Priority: Major
Reporter: Alf Kristian Støyle Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Moving-the-require-into-the-ns-declaration-makes-tes.patch    

 Description   

The tools.nrepl build has been failing since April 18th (http://build.clojure.org/job/tools.nrepl/269/console):

[INFO] Exception in thread "main" java.lang.RuntimeException: Unable to resolve var: clojure.pprint/use-method in this context, compiling:(clojure/tools/nrepl/server.clj:110:3)
[INFO] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6651)
[INFO] 	at clojure.lang.Compiler.analyze(Compiler.java:6445)
[INFO] 	at clojure.lang.Compiler.analyze(Compiler.java:6406)
[INFO] 	at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3665)
[INFO] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6646)
[INFO] 	at clojure.lang.Compiler.analyze(Compiler.java:6445)
[INFO] 	at clojure.lang.Compiler.analyze(Compiler.java:6406)
[INFO] 	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5782)
[INFO] 	at clojure.lang.Compiler$TryExpr$Parser.parse(Compiler.java:2191)
[INFO] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6644)
[INFO] 	at clojure.lang.Compiler.analyze(Compiler.java:6445)
[INFO] 	at clojure.lang.Compiler.analyze(Compiler.java:6406)
[INFO] 	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5782)
[INFO] 	at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5217)
[INFO] 	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3846)
[INFO] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6642)
[INFO] 	at clojure.lang.Compiler.analyze(Compiler.java:6445)
[INFO] 	at clojure.lang.Compiler.eval(Compiler.java:6700)
[INFO] 	at clojure.lang.Compiler.load(Compiler.java:7130)
[INFO] 	at clojure.lang.RT.loadResourceScript(RT.java:370)
[INFO] 	at clojure.lang.RT.loadResourceScript(RT.java:361)
[INFO] 	at clojure.lang.RT.load(RT.java:440)
[INFO] 	at clojure.lang.RT.load(RT.java:411)
[INFO] 	at clojure.core$load$fn__5066.invoke(core.clj:5641)
[INFO] 	at clojure.core$load.doInvoke(core.clj:5640)
[INFO] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
[INFO] 	at clojure.core$load_one.invoke(core.clj:5446)
[INFO] 	at clojure.core$load_lib$fn__5015.invoke(core.clj:5486)
[INFO] 	at clojure.core$load_lib.doInvoke(core.clj:5485)
[INFO] 	at clojure.lang.RestFn.applyTo(RestFn.java:142)
[INFO] 	at clojure.core$apply.invoke(core.clj:626)
[INFO] 	at clojure.core$load_libs.doInvoke(core.clj:5528)
[INFO] 	at clojure.lang.RestFn.applyTo(RestFn.java:137)
[INFO] 	at clojure.core$apply.invoke(core.clj:626)
[INFO] 	at clojure.core$require.doInvoke(core.clj:5607)
[INFO] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
[INFO] 	at clojure.tools.nrepl_test$eval5$loading__4958__auto____6.invoke(nrepl_test.clj:1)
[INFO] 	at clojure.tools.nrepl_test$eval5.invoke(nrepl_test.clj:1)
[INFO] 	at clojure.lang.Compiler.eval(Compiler.java:6703)
[INFO] 	at clojure.lang.Compiler.eval(Compiler.java:6692)
[INFO] 	at clojure.lang.Compiler.load(Compiler.java:7130)
[INFO] 	at clojure.lang.RT.loadResourceScript(RT.java:370)
[INFO] 	at clojure.lang.RT.loadResourceScript(RT.java:361)
[INFO] 	at clojure.lang.RT.load(RT.java:440)
[INFO] 	at clojure.lang.RT.load(RT.java:411)
[INFO] 	at clojure.core$load$fn__5066.invoke(core.clj:5641)
[INFO] 	at clojure.core$load.doInvoke(core.clj:5640)
[INFO] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
[INFO] 	at clojure.core$load_one.invoke(core.clj:5446)
[INFO] 	at clojure.core$load_lib$fn__5015.invoke(core.clj:5486)
[INFO] 	at clojure.core$load_lib.doInvoke(core.clj:5485)
[INFO] 	at clojure.lang.RestFn.applyTo(RestFn.java:142)
[INFO] 	at clojure.core$apply.invoke(core.clj:626)
[INFO] 	at clojure.core$load_libs.doInvoke(core.clj:5524)
[INFO] 	at clojure.lang.RestFn.applyTo(RestFn.java:137)
[INFO] 	at clojure.core$apply.invoke(core.clj:626)
[INFO] 	at clojure.core$require.doInvoke(core.clj:5607)
[INFO] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
[INFO] 	at user$eval1.invoke(run-test6023873211541663255.clj:1)
[INFO] 	at clojure.lang.Compiler.eval(Compiler.java:6703)
[INFO] 	at clojure.lang.Compiler.load(Compiler.java:7130)
[INFO] 	at clojure.lang.Compiler.loadFile(Compiler.java:7086)
[INFO] 	at clojure.main$load_script.invoke(main.clj:274)
[INFO] 	at clojure.main$script_opt.invoke(main.clj:336)
[INFO] 	at clojure.main$main.doInvoke(main.clj:420)
[INFO] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
[INFO] 	at clojure.lang.Var.invoke(Var.java:379)
[INFO] 	at clojure.lang.AFn.applyToHelper(AFn.java:154)
[INFO] 	at clojure.lang.Var.applyTo(Var.java:700)
[INFO] 	at clojure.main.main(main.java:37)
[INFO] Caused by: java.lang.RuntimeException: Unable to resolve var: clojure.pprint/use-method in this context
[INFO] 	at clojure.lang.Util.runtimeException(Util.java:221)
[INFO] 	at clojure.lang.Compiler$TheVarExpr$Parser.parse(Compiler.java:659)
[INFO] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6644)
[INFO] 	... 69 more

This is for the version

  • clojure-1.3.0
  • clojure-1.4.0
  • clojure-1.5.0
  • clojure-1.6


 Comments   
Comment by Alf Kristian Støyle [ 04/Aug/14 1:13 PM ]

Seems the require functions is "not called correctly" when inside the try special form. Moving it to top level, or into the ns declaration resolves the issue. Attached a patch.

Comment by Chas Emerick [ 19/Aug/14 12:36 PM ]

Yeah, whoops. I think I did that thinking I was still trying to maintain Clojure 1.1 compatibility. Thanks for the patch, but I had already fixed it locally.





[NREPL-56] Test suite failing on JDK 1.8 Created: 23/May/14  Updated: 19/Aug/14  Resolved: 19/Aug/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.3
Fix Version/s: 0.2.4

Type: Defect Priority: Major
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

e.g. http://build.clojure.org/job/tools.nrepl-test-matrix/154/CLOJURE_VERSION=1.6.0,jdk=Oracle%20JDK%201.8/console



 Comments   
Comment by Chas Emerick [ 19/Aug/14 12:38 PM ]

Thanks for the heads-up, fixed in 9c41e346053d6c3dbd7ae127b37b6582f894e9d2





[NREPL-58] Inconsistency between session and eval responses Created: 04/Jun/14  Updated: 19/Aug/14  Resolved: 19/Aug/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.4
Fix Version/s: 0.2.4

Type: Defect Priority: Major
Reporter: Jeff Valk Assignee: Chas Emerick
Resolution: Completed Votes: 1
Labels: bug, patch,

Attachments: Text File 0001-eval-session-consistency.patch    
Patch: Code

 Description   

At present, the interruptible-eval handler sends the :value or :eval-error status message before it updates the session to reflect the new binding result. This leaves the session values, particularly *e, *1, etc inconsistent with the state implied by those response messages.

As a practical example, if we want to wrap the "eval" op to check for an :eval-error and add error detail to the response from *e, (get @session #'*e) needs to hold the exception that resulted in the :eval-error. At present, however, it still holds the previous exception, leaving us no way to retrieve the current one in band.

The attached (minimal) patch makes ensures the session is updated before "eval" responses are sent.



 Comments   
Comment by Jeff Valk [ 04/Jun/14 1:00 PM ]

In the event that the attached patch seems a reasonable fix, I should probably add that my CA will be in the mail shortly.

Comment by Bozhidar Batsov [ 13/Jun/14 8:05 AM ]

I'm just here to pledge my support for Jeff's patch. Btw, Jeff, few days later and you could have signed the CA electronically.

Comment by Jeff Valk [ 13/Jun/14 9:32 AM ]

The irony did not escape me. Anyway, my CA (in dead tree form) is now on file.

Comment by Chas Emerick [ 19/Aug/14 1:56 PM ]

Looks good, thanks for the patch Applied as f02956d.





[NREPL-40] Thread leak in clojure.tools.nrepl.transport$fn_transport? Created: 11/Mar/13  Updated: 19/Aug/14  Resolved: 19/Aug/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.1, 0.2.2
Fix Version/s: 0.2.4

Type: Defect Priority: Major
Reporter: David Lao Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None
Environment:

Windows 7 x64, Oracle JDK 1.7.0.11 x64, clojure 1.4.0



 Description   

When trying out remote eval using your sample in the README, ie

(with-open [conn (repl/connect :port 59258)]
(-> (repl/client conn 1000) ; message receive timeout required
(repl/message {:op "eval" :code "(+ 2 3)"})
repl/response-values))

I'm noticing that hosting process leaking a thread each time the remote eval is called. Jconsole shows a clojure-agent-send-off-pool-xxx thread got spawn as result of the call. The stack appears to be pointing to the "while true" loop inside fn_transport.

java.util.concurrent.locks.LockSupport.park(LockSupport.java:186)
java.util.concurrent.SynchronousQueue$TransferStack.awaitFulfill(SynchronousQueue.java:458)
java.util.concurrent.SynchronousQueue$TransferStack.transfer(SynchronousQueue.java:359)
java.util.concurrent.SynchronousQueue.put(SynchronousQueue.java:878)
clojure.tools.nrepl.transport$fn_transport$fn__3912.invoke(transport.clj:44)
clojure.core$binding_conveyor_fn$fn__3989.invoke(core.clj:1819)
clojure.lang.AFn.call(AFn.java:18)
java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:334)
java.util.concurrent.FutureTask.run(FutureTask.java:166)
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1110)
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:603)
java.lang.Thread.run(Thread.java:722)

What do you recommend as way to free the thread? I have server code that calls nrepl on behave of client connections and the number of call can pile up fairly quickly.



 Comments   
Comment by David Lao [ 12/Mar/13 3:37 PM ]

Here is my workaround.

— a/src/main/clojure/clojure/tools/nrepl/transport.clj
+++ b/src/main/clojure/clojure/tools/nrepl/transport.clj
@@ -36,12 +36,12 @@
to the 2 or 3 functions provided."
([read write] (fn-transport read write nil))
([read write close]

  • (let [read-queue (SynchronousQueue.)]
  • (future (try
  • (while true
  • (.put read-queue (read)))
  • (catch Throwable t
  • (.put read-queue t))))
    + (let [read-queue (SynchronousQueue.)
    + transport-thread (future (try
    + (while true
    + (.put read-queue (read)))
    + (catch Throwable t
    + (.put read-queue t))))]
    (FnTransport.
    (let [failure (atom nil)]
    #(if @failure
    @@ -51,7 +51,7 @@
    (do (reset! failure msg) (throw msg))
    msg))))
    write
  • close))))
    + (fn [] (close)(future-cancel transport-thread))))))
Comment by Chas Emerick [ 06/Aug/13 5:47 AM ]

Very nice catch. I'd love to have a patch from you; do you have a CA filed? (see http://clojure.org/contributing)

Comment by Chas Emerick [ 18/Apr/14 10:07 AM ]

Ping: did you send in a CA? Would love to apply a patch from you for this.

Comment by Chas Emerick [ 19/Aug/14 8:43 PM ]

I wanted to get this out as part of 0.2.4, so implemented my own patch, committed as 20a3d15. Thanks for the report!





[NREPL-65] Clients have no way to signal EOF when reading stdin Created: 24/Aug/14  Updated: 04/Sep/14  Resolved: 24/Aug/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.4
Fix Version/s: 0.2.5

Type: Defect Priority: Major
Reporter: Colin Jones Assignee: Colin Jones
Resolution: Completed Votes: 0
Labels: None


 Description   

As described in https://github.com/technomancy/leiningen/issues/1658

The client ought to be able to signal an EOF by sending a message like {:op :stdin :stdin nil}.



 Comments   
Comment by Colin Jones [ 24/Aug/14 11:00 PM ]

Closed by https://github.com/clojure/tools.nrepl/commit/eb526fd8498ced1b4bd1555f8ff680f3ad65f1b4

Comment by Chas Emerick [ 25/Aug/14 7:50 AM ]

Good, thanks!





[NREPL-66] *msg* is captured on first print Created: 10/Sep/14  Updated: 10/Sep/14  Resolved: 10/Sep/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.5
Fix Version/s: 0.2.6

Type: Defect Priority: Major
Reporter: Hugo Duncan Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File fix_msg_capture.patch    

 Description   

The `msg` var is captured on first print by `interruptable-eval/evaluate`.

This means that all output is sent back to the client with the message id of the first message that prints.

See https://github.com/hugoduncan/tools.nrepl/commit/fcff5c8daa649a4c3996aa47d6132a3bb0ca77ce for a fix.



 Comments   
Comment by Hugo Duncan [ 10/Sep/14 8:47 AM ]

I think it affects 0.2.4 as well.

Comment by Hugo Duncan [ 10/Sep/14 8:53 AM ]

Patch from git commit above.

Comment by Chas Emerick [ 10/Sep/14 9:37 AM ]

Thanks!





[NREPL-72] eval middleware messes up *ns* Created: 11/Jan/15  Updated: 12/Jan/15  Resolved: 12/Jan/15

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.5
Fix Version/s: 0.2.7

Type: Defect Priority: Major
Reporter: Bozhidar Batsov Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

When doing eval requests the responses echo the ns of the request instead of the *ns* (unless you're evaluating in-ns or ns). This is not consistent with the eval behaviour in piggieback.

The details of the problem are discussed here:



 Comments   
Comment by Chas Emerick [ 12/Jan/15 8:18 AM ]

Fixed @ b905245, available in 0.2.7-SNAPSHOT via the Sonatype OSS snapshots/public repository.





[NREPL-71] reflection warnings on 0.2.5 Created: 05/Dec/14  Updated: 12/Jan/15  Resolved: 12/Jan/15

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.5
Fix Version/s: 0.2.7

Type: Defect Priority: Major
Reporter: Jay Fields Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File refwarn.patch    

 Description   

After upgrading to 0.2.5 I now get the following reflection warnings when I use tools.nrepl

[08:00:22][Step 1/1] Reflection warning, clojure/tools/nrepl/ack.clj:47:3 - reference to field close can't be resolved.
[08:00:22][Step 1/1] Reflection warning, clojure/tools/nrepl/server.clj:140:28 - call to java.net.InetSocketAddress ctor can't be resolved.

My app is pretty perf sensitive, so we fail the build on any Reflection warning - thus the importance to my team is probably higher than it would be to the average developer.



 Comments   
Comment by Jay Fields [ 05/Dec/14 8:21 AM ]

just tested with 0.2.6, same issue.

Comment by Jay Fields [ 05/Dec/14 12:07 PM ]

Here's a patch with the changes I made locally. Seems to work for me, but you'll probably want to check it out just to be safe.

Comment by Chas Emerick [ 12/Jan/15 8:42 AM ]

Fixed on master. I'm afraid I couldn't apply your patch because your name is not listed @ http://clojure.org/contributing





[NREPL-68] load-file op responses always have "user" or ":main" as the value of their ns slot Created: 22/Sep/14  Updated: 12/Mar/15  Resolved: 12/Mar/15

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.5
Fix Version/s: 0.2.8

Type: Defect Priority: Major
Reporter: Bozhidar Batsov Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

Regardless of the ns of the file being loaded by load-file, its response always has "user" or ":main" as the value of its ns slot. Seems much more logical to me to return the ns of the file that was loaded.

On a related note - the return value for this op does not appear in the op reference.



 Comments   
Comment by Chas Emerick [ 22/Sep/14 4:58 AM ]

load-file reuses the default handler for eval, so the returned namespace is going to be *ns*. I just confirmed this by fiddling with one of nREPL's load-file tests.

Perhaps cider is not sending the session ID on load-file messages?

Comment by Bozhidar Batsov [ 22/Sep/14 5:05 AM ]

Seems to me we're sending it. The relevant code is here.

Comment by Kevin Downey [ 23/Sep/14 7:21 PM ]

the compiler binds and pops the value of ns when loading, so by the time eval captures the value of ns the compiler has already loaded the file and popped the value.

Comment by Chas Emerick [ 12/Jan/15 8:38 AM ]

Looking at this again, seems to me that load-file shouldn't include *ns* in its response at all (since loading a file should not affect *ns* anyway). That it does is just a side effect of the implementation detail of using interruptible-eval to effect the loading of the code in question.

Comment by Bozhidar Batsov [ 12/Jan/15 12:05 PM ]

I'm perfectly fine with dropping the ns from the response. That would probably be best indeed.

Comment by Chas Emerick [ 12/Mar/15 9:33 AM ]

load-file now elides :ns in its response messages, https://github.com/clojure/tools.nrepl/commit/ece146671





[NREPL-67] nrepl-server reuseAddr Created: 19/Sep/14  Updated: 12/Mar/15  Resolved: 12/Mar/15

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.5
Fix Version/s: 0.2.8

Type: Enhancement Priority: Major
Reporter: Sun Ning Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

We are using nrepl-server widely in our application. A pain with nrepl-server is it's not configured to reuseAddr, so we often failed to restart server due to port conflicting.

So I hope you can enable reuseAddr by default for the server socket. Thanks!



 Comments   
Comment by Chas Emerick [ 12/Jan/15 8:39 AM ]

Patch welcome. Thanks!

Comment by Chas Emerick [ 12/Mar/15 9:50 AM ]

Fixed on master, https://github.com/clojure/tools.nrepl/commit/db9125c





[NREPL-7] Dependency on 1.3.0-master-SNAPSHOT instead of 1.3.0 Created: 15/Nov/11  Updated: 15/Nov/11  Resolved: 15/Nov/11

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Steve Lindsay Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

Running "mvn install" on tools.nrepl results in a build failure:

[INFO] Missing:
[INFO] ----------
[INFO] 1) org.clojure:clojure:jar:1.3.0-master-SNAPSHOT

Changing 1.3.0-master-SNAPSHOT to 1.3.0 in src/integration/clojure-1.3.0/pom.xml seems to fix it.



 Comments   
Comment by Chas Emerick [ 15/Nov/11 5:13 AM ]

Bizarre, since it is building cleanly here and on build.clojure.org.

I should certainly update the version number in that pom to 1.3.0 though. I'll do that shortly.

FWIW, maybe adding a -U to your mvn invocation would help? That forces a check of all snapshots.

Comment by Chas Emerick [ 15/Nov/11 5:22 AM ]

fixed on HEAD





[NREPL-39] Using functions reading from *in* causes "java.io.IOException: Write end dead" Created: 03/Feb/13  Updated: 07/Apr/13  Resolved: 21/Mar/13

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Adrian Bendel Assignee: Colin Jones
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Use-a-queue-for-stdin-to-avoid-PipedReader-Writer.patch    

 Description   

When calling (read-line) from lein repl or ccw repl, waiting around four minutes (on my machine),
evaluating any form causes the following exception:

ERROR: Unhandled REPL handler exception processing message {:code (+ 1 1), :id 08e30b70-ab0f-4a47-8937-3c040ce06f4f, :ns user, :op eval, :session 916200cc-e476-4ac4-98f6-76057a9020be}
java.io.IOException: Write end dead
	at java.io.PipedReader.ready(Unknown Source)
	at clojure.tools.nrepl.middleware.session.proxy$java.io.PipedReader$0.ready(Unknown Source)
	at java.io.BufferedReader.ready(Unknown Source)
	at java.io.FilterReader.ready(Unknown Source)
	at java.io.PushbackReader.ready(Unknown Source)
	at clojure.tools.nrepl.middleware.session$add_stdin$fn__1103.invoke(session.clj:197)
	at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__840.invoke(middleware.clj:17)
	at clojure.tools.nrepl.middleware.session$session$fn__1093.invoke(session.clj:165)
	at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__840.invoke(middleware.clj:17)
	at clojure.tools.nrepl.server$handle_STAR_.invoke(server.clj:18)
	at clojure.tools.nrepl.server$handle$fn__1132.invoke(server.clj:27)
	at clojure.core$binding_conveyor_fn$fn__4130.invoke(core.clj:1836)
	at clojure.lang.AFn.call(AFn.java:18)
	at java.util.concurrent.FutureTask$Sync.innerRun(Unknown Source)
	at java.util.concurrent.FutureTask.run(Unknown Source)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
	at java.lang.Thread.run(Unknown Source)

While googling a bit on this and without concrete knowledge of the implementation in nrepl,
it seems like closing a thread that wrote to PipedWriter, before closing the writer or writing to it from another thread,
causes PipedReader#ready to throw that exception.



 Comments   
Comment by Colin Jones [ 06/Mar/13 5:46 PM ]

I found this related article: http://techtavern.wordpress.com/2008/07/16/whats-this-ioexception-write-end-dead/

So it seems like we may need something a little more robust. I'm thinking just a LinkedBlockingQueue that gets written to on input, with a Reader proxy over the top to hook up to Clojure's LineNumberingPushbackReader. I've started fiddling with that idea, passing most of the tests already, and I think this is the right path to go down in general. Yell at me if you see a problem with this plan.

Comment by Chas Emerick [ 07/Mar/13 4:50 AM ]

I had no idea that PipedReader & co. actually track the threads that touch them. That makes them entirely inappropriate for the *in* case.

+1 to your plan, thanks for chasing this!

Comment by Colin Jones [ 08/Mar/13 3:28 PM ]

OK, this works for me against REPLy.

I'd have preferred to avoid the dynamic binding (skipping-eol), but I haven't been able to think of another way to accomplish what it does. The problem is that we don't want to send another :need-input message just to clear newlines, but we don't have any visibility into what's in the LineNumberingPushbackReader's buffer (neither the full deal, nor the pushback). My initial thought was to replace the LineNumberingPushbackReader directly, with a custom thing, but I think the assumptions around that class in `read` and `read-line` are too concretely baked in to allow it (especially before 1.5, when the buffer size became configurable).

And there's no room for adding bits to the Reader interface, so in order to effect a behavior difference in the Reader, depending on what the high-level state is, I don't know that there are any options aside from dynamic binding or an atom (and of the two, a var seemed more robust, no worries about finally & friends).

Comment by Colin Jones [ 21/Mar/13 11:39 PM ]

Pushed to master: https://github.com/clojure/tools.nrepl/commit/3cd3d0e8b2091fc70fde1587d3f0a995cbf8b283





[NREPL-21] Sometimes *err*/*out*/value messages are surprisingly ordered Created: 17/Jun/12  Updated: 20/Jun/12  Resolved: 20/Jun/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Colin Jones Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Flush-out-err-before-sending-value.patch    
Patch: Code

 Description   

As described here: https://github.com/technomancy/leiningen/issues/631, the *out*, *err*, and value printing are ordered differently than expected sometimes.

There are 2 things left here:
(1) The return value can sometimes print before synchronous prints to err/out. Synchronizing all the printing on the REPLy side doesn't seem to help. An example case:

=> (do (.print *err* "one") (print "two") (print "three"))
niltwothreeone

(2) Calling .println() on the *out*/*err* streams doesn't necessarily flush the stream. Changing this may have performance/network ramifications, but generally it seems like surprising behavior. An example case:

user=> (do (.println *out* "one") (.print *err* "two") (.print *err* "three") 1)
twothreeone
1

I'd expect two .print() calls to different streams to have undefined ordering, but the other orderings were all surprising.

The attached patch forces flushes to avoid both of these, for (1) immediately before sending the value message, and for (2) on the "normal" PrintStream [autoFlush] cases of println, printf, or format.

Side note: I couldn't get any tests to fail in this scenario, even after doing some reworking of the streams used in tests, but I kept one new test in this patch anyway. Turns out it's hard to write tests that really make sure .flush() gets called.



 Comments   
Comment by Chas Emerick [ 20/Jun/12 6:58 AM ]

Merged, thanks!





[NREPL-18] Flush *out*/*err* after N characters Created: 18/Apr/12  Updated: 20/Jun/12  Resolved: 20/Jun/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Colin Jones Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Flush-output-after-session-out-limit-characters.patch    
Patch: Code and Test

 Description   

Not sure how this should be configured, but it would be nice if doing something like (println large-seq-here) would flush output automatically after some number of characters. This is so that output could stream constantly across the nrepl connection instead of waiting for the entire output to send as one message (assuming no manual flushes).



 Comments   
Comment by Colin Jones [ 28/Apr/12 1:31 PM ]

There is a precedent for this, in the sense that many output buffers have some fixed size, and they are flushed when that size has been reached (e.g. http://www.gnu.org/software/libc/manual/html_node/Flushing-Buffers.html, http://docs.oracle.com/javase/6/docs/api/java/io/BufferedWriter.html)

We had talked about potential worries / headaches, where users may get partial lines printed at a time. But after more thought about it, I'm thinking we may be OK: concurrent out/err writes can already interleave in interesting ways, and depending on the placement of (flush) calls (explicitly or implicitly) to make them more atomic is something I'd already have expected to be fragile. Maybe others will see things I don't, though.

The implementation I have uses 1024 as the default, but that's pretty arbitrary. A less dynamic way of setting the limit would be also be fine with me, though since we're already in I/O-land, I'd assume performance isn't going to be bound by the dynamic var lookup.

Comment by Colin Jones [ 17/Jun/12 6:09 PM ]

Any thoughts on this one? It would be great to be able to print big seqs incrementally.

Comment by Chas Emerick [ 20/Jun/12 7:39 AM ]

Sorry for the delay, this fell through the cracks.

Looks good to me, except: can we make the "buffer size" an argument to the session middleware fn? That's where people will want to be able to customize it, if ever.

Commit and close this at will.

Comment by Colin Jones [ 20/Jun/12 11:14 PM ]

Done, thanks a bunch!





[NREPL-35] Eliminate several uses of reflection in tools.nrepl Created: 28/Oct/12  Updated: 31/Oct/12  Resolved: 31/Oct/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.0

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File nrepl-35-eliminate-reflection-v1.txt    

 Description   

There are several uses of reflection in tools.nrepl that can be eliminated with suitable type hints.



 Comments   
Comment by Andy Fingerhut [ 28/Oct/12 10:12 PM ]

nrepl-35-eliminate-reflection-v1.txt dated Oct 28 2012 adds type hints to eliminate some of the reflection warnings in tools.nrepl. There are still some left over that are not as obvious how to eliminate, if it is even possible.

Comment by Chas Emerick [ 31/Oct/12 4:44 AM ]

Thanks for the patch, but it overhinted in some areas (I try to keep hints at an absolute minimum to maximize readability) and overconstrained in others (e.g. ThreadPoolExecutor vs. Executor).

Fixed @ https://github.com/clojure/tools.nrepl/commit/ac95dc7da973f3d7edea75d0f6f9ce01ee18641d





[NREPL-37] Printing reference returned by clojure.tools.nrepl.server/start-server causes multimethod exception Created: 20/Dec/12  Updated: 26/Feb/13  Resolved: 26/Feb/13

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.0-RC1
Fix Version/s: 0.2.2

Type: Defect Priority: Minor
Reporter: Vaughn Dickson Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: bug


 Description   

I was accidentally printing the reference returned by start-server by calling it as the final function in my main method, which causes this error:

java.lang.IllegalArgumentException: Multiple methods in multimethod 'print-method' match dispatch value: class clojure.tools.nrepl.server.Server -> interface clojure.lang.IDeref and interface clojure.lang.IRecord, and neither is preferred
at clojure.lang.MultiFn.findAndCacheBestMethod(MultiFn.java:136)
at clojure.lang.MultiFn.getMethod(MultiFn.java:111)
at clojure.lang.MultiFn.getFn(MultiFn.java:119)
at clojure.lang.MultiFn.invoke(MultiFn.java:167)
at clojure.core$pr_on.invoke(core.clj:3266)
at clojure.core$pr.invoke(core.clj:3278)
at clojure.lang.AFn.applyToHelper(AFn.java:161)
at clojure.lang.RestFn.applyTo(RestFn.java:132)
at clojure.core$apply.invoke(core.clj:601)
at clojure.core$prn.doInvoke(core.clj:3311)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.main$eval_opt.invoke(main.clj:299)
at clojure.main$initialize.invoke(main.clj:316)
at clojure.main$null_opt.invoke(main.clj:349)
at clojure.main$main.doInvoke(main.clj:427)
at clojure.lang.RestFn.invoke(RestFn.java:421)
at clojure.lang.Var.invoke(Var.java:419)
at clojure.lang.AFn.applyToHelper(AFn.java:163)
at clojure.lang.Var.applyTo(Var.java:532)
at clojure.main.main(main.java:37)



 Comments   
Comment by Chas Emerick [ 02/Feb/13 7:35 AM ]

The fact that the returned record is also a clojure.lang.IDeref is a temporary compatibility measure, a result of changing to returning a record.

Adding a print-method implementation for the Server type that explicitly delegates to the IRecord implementation would resolve the problem.

Comment by Chas Emerick [ 26/Feb/13 4:42 AM ]

Fixed @ http://github.com/clojure/tools.nrepl/commit/0f016eb





[NREPL-22] CLONE - set stack size for Android Created: 30/Jun/12  Updated: 24/Jul/12  Resolved: 24/Jul/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.0.5
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Hank Assignee: Chas Emerick
Resolution: Duplicate Votes: 0
Labels: Android
Environment:

Android



 Description   

If you want to use nREPL on Android (currently only possible with sattvik's fork of Clojure) then you need bigger stack sizes than Android's default 8k to eval even moderately complex expressions. For comparison: A regular JVM on a PC has default stack sizes around 256k ... 512k depending on platform.

In nrepl.clj, modify

(doto (Thread. r)
(.setDaemon true))))))

to read

(doto (Thread. (.getThreadGroup (Thread/currentThread)) r "nREPL" 524288) ; 512k generous stack size
(.setDaemon true))))))

Note: There are warnings here http://bit.ly/u86tF1 about choosing a good stack size but since in practice nREPL seems to have <10 threads running with 1 active client connection we can be generous here.

The more advanced version would be to make this somehow configurable but that'd probably be over-engineering things.



 Comments   
Comment by Hank [ 30/Jun/12 9:21 AM ]

I understand that after a brief detour via agents we're now back to creating our own threads in nREPL. So the above applies gain. Apparently in JIRA you can't reopen issues so I'm 'cloning' this one.

Comment by Chas Emerick [ 24/Jul/12 4:52 AM ]

I was able to reopen NREPL-8; let's track it there.





[NREPL-41] Rebind print-.* dynamic vars when loading a file Created: 21/Mar/13  Updated: 21/Mar/13  Resolved: 21/Mar/13

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Colin Jones Assignee: Colin Jones
Resolution: Completed Votes: 0
Labels: None


 Description   

Syntax errors happen when the user has bound things like *print-length*, *print-depth* and those take precedence.

See https://github.com/technomancy/leiningen/issues/952

Based on discussion in that ticket, I think the right approach is to rebind these vars when doing pr-str in load_file.clj - but I haven't yet been able to reproduce locally yet.



 Comments   
Comment by Colin Jones [ 21/Mar/13 11:38 PM ]

fixed on master: https://github.com/clojure/tools.nrepl/commit/11e3993746950640cb8173fbcd3b074c53b26ce0





[NREPL-9] (read-line) doesn't prompt for input after a (read) Created: 19/Feb/12  Updated: 21/Feb/12  Resolved: 21/Feb/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Colin Jones Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

When (read-line) is preceded by (read), it already has a newline in the stream, and returns "" immediately rather than prompting for input.

(read) then (read-line) works fine in the stock Clojure repl because the REPL shares STDIN with the in used by the normal (read) and (read-line) calls. So when the REPL's loop does clojure.main/repl-read (to get my evaluation input "(read-line)"), it skips whitespace before the read for the thing to eval, reads that thing, and then skips a newline, if that's the next thing: https://github.com/clojure/clojure/blob/master/src/clj/clojure/main.clj#L145-161

That allows the .read calls generated by the (read-line) execution to start after a trailing newline left in the stream.

We can do the same thing in nrepl.middleware.session/add-stdin on an eval message, but we need to check .ready first, to avoid sending unnecessary :need-input requests back to the client. I think this does belong in add-stdin, since it's essentially a stdin concern, even though it acts on the eval message.

I've got one test exposing the problem, and one proving we only eat newlines, not any old trailing character. I had to stick :stdin-reader on the session to enable consuming the newline from the middleware - not sure whether there's a way to do without it that I'm not seeing.



 Comments   
Comment by Chas Emerick [ 21/Feb/12 5:17 AM ]

Thanks for chasing this down! Patch applied to master.





[NREPL-10] SocketExceptions on transport/bencode when the other end has gone away Created: 19/Feb/12  Updated: 12/Oct/12  Resolved: 12/Oct/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.0, 0.2.0-RC1

Type: Defect Priority: Minor
Reporter: Colin Jones Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

When running the tests, I'm occasionally getting SocketExceptions (which don't fail the test suite) with the message "Broken pipe". I think these stem from one end of the nREPL socket trying to write to the socket after the other end has gone away.

There's an existing test (ensure-closeable) that explicitly expects a SocketException to be raised if the socket has had .close called directly on it, rather than having the other end hang up, which I think is what the "Broken pipe" tells us. I'm curious as to whether this is important for some reason I haven't seen, or whether catching all SocketExceptions on transport/bencode's write function and returning nil would be appropriate.

If the end doing the socket closing actually should affect the behavior, we could sniff the exception message, but that feels a bit yucky.

I can actually reproduce this in "real life" by firing up an nREPL server / client (`reply --nrepl --port 9999`), then attaching to it (`reply --nrepl --attach 9999`), and in the attached client, run (Thread/sleep 10000) and immediately kill the process. Then the "Broken pipe" exception shows up in the original REPL with the server in the same process.

I've got a patch for this in my bitbucket fork (socket-exception branch): https://bitbucket.org/trptcolin/nrepl/changeset/4a432f3ce95a



 Comments   
Comment by Chas Emerick [ 21/Feb/12 5:35 AM ]

What do we really want to happen when a SocketException (or really, any error) occurs?

I'm all for better error handling/reporting, but it shouldn't obfuscate what's going wrong. Catching and squelching isn't doing anyone any favors. That could be particularly confusing if done at the transport level. "You mean all the messages I've been sending have been dropped on the floor silently, and I was supposed to be checking for a nil return?"

Comment by Colin Jones [ 26/Feb/12 12:46 PM ]

Yeah, you're right. Although, isn't checking for a nil return already necessary to account for timeouts?

What about having a server-side transport error handler that's rebindable, and re-throws by default? Or maybe we could collect errors somewhere, similar to what clojure.core/agent-error does.

The more I think about it, the less important this seems, though - mainly an annoyance when running tests.

Comment by Chas Emerick [ 12/Oct/12 12:51 AM ]

Was simply a disconnection stacktrace due to a client closing up prior to an evaluation response being written to the transport.

Fixed in https://github.com/clojure/tools.nrepl/commit/2d90bdd4fe14298bbfcaab52ecdea48781c71604





[NREPL-8] set stack size for Android Created: 30/Dec/11  Updated: 02/Feb/14  Resolved: 03/Oct/12

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.0.5
Fix Version/s: 0.2.0-beta9

Type: Enhancement Priority: Minor
Reporter: Hank Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: Android
Environment:

Android



 Description   

If you want to use nREPL on Android (currently only possible with sattvik's fork of Clojure) then you need bigger stack sizes than Android's default 8k to eval even moderately complex expressions. For comparison: A regular JVM on a PC has default stack sizes around 256k ... 512k depending on platform.

In nrepl.clj, modify

(doto (Thread. r)
(.setDaemon true))))))

to read

(doto (Thread. (.getThreadGroup (Thread/currentThread)) r "nREPL" 524288) ; 512k generous stack size
(.setDaemon true))))))

Note: There are warnings here http://bit.ly/u86tF1 about choosing a good stack size but since in practice nREPL seems to have <10 threads running with 1 active client connection we can be generous here.

The more advanced version would be to make this somehow configurable but that'd probably be over-engineering things.



 Comments   
Comment by Chas Emerick [ 30/Dec/11 9:14 AM ]

Noted, thank you. I'll try to make sure a provision for this gets into the next significant release.

My first inclination is to ensure that the stack size is configurable; a hardcoded size would likely be just as inappropriate in some environments as the system default.

Comment by Chas Emerick [ 21/Feb/12 5:21 AM ]

nREPL is now using Clojure's built-in agent threadpools. This should theoretically simplify things for you, correct? i.e. Isn't a patched build of Clojure necessary for use on Android for this stack size issue, among other reasons?

Comment by Hank [ 22/Feb/12 7:21 AM ]

The patched build is only necessary for the JVM byte code that the Clojure compiler dynamically generates to be converted to Dalvik bytecode. However adding additional android patches such as configuring the agent threadpool with a larger stack size do make sense. I'm not the official Android Clojure maintainer though – I guess there isn't an official one really – so my opinion on this only goes so far.

Comment by Chas Emerick [ 22/Feb/12 8:11 AM ]

Well, I think you have a solid case for expanding the scope of the android fork, or even for a change to parameterize the threadpools in Clojure itself.

I'm going to close this; there's really nothing to be done from within nREPL given that it doesn't maintain its own threadpool anymore.

Comment by Chas Emerick [ 24/Jul/12 4:49 AM ]

Reopening; since the use of agents was a fool's errand, parameterizing stack size needs to happen.

Comment by Chas Emerick [ 15/Aug/12 3:58 PM ]

Now that I look at this again, there are a number of options in nREPL to solve this.

First, you can provide an :executor argument to the interruptible-eval middleware. This could be any java.util.concurrent.Executor, so you can configure it to use whatever threads you want.

Second, and if you want to minimize work and retain as much of nREPL's default configuration otherwise, you can patch in your own thread pool by binding #'clojure.tools.nrepl.middleware.interruptible-eval/configure-thread-factory like so:

(def thread-group ...)
(def stack-size ...)

(defn android-thread-factory
  "Returns a new ThreadFactory suitable for use with android. This implementation
   generates daemon threads, with names that include the session id."
  []
  (let [session-thread-counter (AtomicLong. 0)]
    (reify ThreadFactory
      (newThread [_ runnable]
        (doto (Thread. thread-group
                runnable
                (format "nREPL-worker-%s" (.getAndIncrement session-thread-counter))
                stack-size)
          (.setDaemon true))))))

(with-bindings {#'clojure.tools.nrepl.middleware.interruptible-eval/configure-thread-factory
                android-thread-factory}
  (clojure.tools.nrepl.server/start-server ...))

The above totally untested, but I hope you get the idea. Give it a shot, and let me know how it goes. I'm going to take off this issue's fix target for now.

Comment by Chas Emerick [ 03/Oct/12 7:03 AM ]

I hear good things at this point about people connecting to nREPL endpoints running on Android. Please file another issue if this or other Android-specific problems arise in the future.

Comment by Jürgen Hötzel [ 23/Jan/14 7:01 AM ]

clojure.tools.nrepl.middleware.interruptible-eval/configure-thread-factory is a private non-dynamic var.

so i had to use with-redefs.

Are there any concerns making this dynamic?

Comment by Chas Emerick [ 23/Jan/14 8:31 AM ]

Not particularly, but with-redefs is a perfectly reasonable option (and likely safer, if you're really looking to patch in a completely different ThreadFactory.

What is prompting you to do so?

Comment by Jürgen Hötzel [ 31/Jan/14 4:08 PM ]

Yes. No need to use dynamic vars.

Comment by Alexander Yakushev [ 02/Feb/14 2:36 AM ]

OK, I finally integrated the fix into Neko. Thank you for help, Chas and Jürgen!





[NREPL-50] Configurable eval function Created: 10/Apr/14  Updated: 18/Apr/14  Resolved: 18/Apr/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.4

Type: Enhancement Priority: Minor
Reporter: Brandon Bloom Assignee: Chas Emerick
Resolution: Completed Votes: 1
Labels: patch, patch,

Attachments: Text File 0001-NREPL-50-Overridable-eval-function.patch    
Patch: Code

 Description   

#'clojure.main/repl has an :eval keyword option that is currently not provided by the interruptable-eval middleware.

I'd like to be able to supply that argument on a per session basis.



 Comments   
Comment by Brandon Bloom [ 10/Apr/14 7:36 PM ]

Attaching patch to support :eval option on (= op :eval) messages.

Comment by Chas Emerick [ 11/Apr/14 7:20 AM ]

This patch adds that option on a per-evaluation basis, which I don't think you really want. Having this set on a per-session basis (as you originally described) makes more sense to me, esp. insofar as doing it that way makes it trivially controllable from "userland" (i.e. the human typing in expressions to be evaluated somewhere). Changing the shape of messages going back and forth requires tool support.

Comment by Brandon Bloom [ 11/Apr/14 9:44 AM ]

Ah, sorry, I should have explained why I ultimately did this at the per-message level. I've got a custom evaluation, but I want to be able to freely mix standard and custom evaluations during the same session. In my case, I have .clj files that use clojure.core/eval and .eclj files that use eclj.core/eval via vim-fireplace. As far as I can tell, Fireplace only opens one session per lein project, so session variables would have been a lot more work.

Is there some way that I can easily override session variables on a per message basis? If so, then I can switch to that to get both the user-level customization and tooling-level support. Otherwise, I'm open to suggestions.

Comment by Chas Emerick [ 12/Apr/14 12:40 PM ]

Sessions are essentially just the result of get-thread-bindings, plus some nREPL bookkeeping. So, as long as evaluate binds e.g. *eval*, users can just set! the var to change it. Though, unless you watch for that in your custom evaluation fn, you'll never be able to change it back. :-P This seems like a good enough reason to keep the option per-message, and require tools/users to ask for what they want each time.

(Tangentially, I just remembered that Piggieback does roughly the same thing you're looking for. I thought about pushing something like what you're suggesting down into nREPL, but I'm very cautious about changing it to suit my needs [myopia and so on]. Glad we have a second real use case, which makes me feel better about the whole thing.)

Comment by Brandon Bloom [ 13/Apr/14 12:09 PM ]

Yeah, set! seems like a surefire way to steamroll over later evaluations unless everybody does a defensive set! all the time, and even then, set! may not even be defined for the evaluator that the caller is expecting!

Maybe this means that there needs to be some sort of general-purpose way to override session variables per message? Then, we'd only need to define eval and all messages could be processed inside their own with-bindings call.

Comment by Chas Emerick [ 14/Apr/14 2:13 PM ]

Maybe, but that's a lot to bite off without having more than "wouldn't it be cool if…" as motivation.

Any thoughts as to whether the eval symbol's namespace should be implicitly {{require}}d?

Will be applying the proposed patch (maybe with a tweak or two) this week.

Comment by Brandon Bloom [ 14/Apr/14 2:23 PM ]

Clojure's standard load-file looks for .class or .clj files. EClj has to patch the loader to look for .eclj files too, and eclj.core may be re-loaded after the new evaluator is bootstrapped.

I have to pre-load the namespace manually either way. Therefore, I don't care if it implicitly requires the namespace, as long as it doesn't :reload the namespace.

Comment by Chas Emerick [ 18/Apr/14 10:04 AM ]

Committed, will be available in 0.2.4-SNAPSHOT momentarily.





[NREPL-51] Pretty-printing reference returned by clojure.tools.nrepl.server/start-server causes multimethod exception Created: 12/Apr/14  Updated: 18/Apr/14  Resolved: 18/Apr/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.3
Fix Version/s: 0.2.4

Type: Defect Priority: Minor
Reporter: Michael Nygard Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: bug


 Description   

I was accidentally printing the reference returned by start-server by calling it as the final function in my main method, which causes this error:

java.lang.IllegalArgumentException: Multiple methods in multimethod 'print-method' match dispatch value: class clojure.tools.nrepl.server.Server -> interface clojure.lang.IDeref and interface clojure.lang.IRecord, and neither is preferred
at clojure.lang.MultiFn.findAndCacheBestMethod(MultiFn.java:136)
at clojure.lang.MultiFn.getMethod(MultiFn.java:111)
at clojure.lang.MultiFn.getFn(MultiFn.java:119)
at clojure.lang.MultiFn.invoke(MultiFn.java:167)
at clojure.core$pr_on.invoke(core.clj:3266)
at clojure.core$pr.invoke(core.clj:3278)
at clojure.lang.AFn.applyToHelper(AFn.java:161)
at clojure.lang.RestFn.applyTo(RestFn.java:132)
at clojure.core$apply.invoke(core.clj:601)
at clojure.core$prn.doInvoke(core.clj:3311)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.main$eval_opt.invoke(main.clj:299)
at clojure.main$initialize.invoke(main.clj:316)
at clojure.main$null_opt.invoke(main.clj:349)
at clojure.main$main.doInvoke(main.clj:427)
at clojure.lang.RestFn.invoke(RestFn.java:421)
at clojure.lang.Var.invoke(Var.java:419)
at clojure.lang.AFn.applyToHelper(AFn.java:163)
at clojure.lang.Var.applyTo(Var.java:532)
at clojure.main.main(main.java:37)



 Comments   
Comment by Michael Nygard [ 12/Apr/14 12:09 PM ]

I cloned this from NREPL-37, but cannot change any of the other fields. Thanks Jira.

Basically, this is the same dispatch problem as NREPL-37 but on clojure.pprint/simple-dispatch.

Version affected is 0.2.3.

Comment by Chas Emerick [ 12/Apr/14 12:21 PM ]

Thanks, should have thought of this when I fixed the related issue.

Comment by Chas Emerick [ 18/Apr/14 10:04 AM ]

Committed, will be available in 0.2.4-SNAPSHOT momentarily.





[NREPL-57] Add Java version info to op "describe"'s response Created: 29/May/14  Updated: 19/Aug/14  Resolved: 19/Aug/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.3
Fix Version/s: 0.2.4

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

Currently the describe op's reply features the relevant nREPL and Clojure versions. It'd be nice if the underlying JVM version was included as well.



 Comments   
Comment by Chas Emerick [ 19/Aug/14 1:45 PM ]

Added in cb7ce41





[NREPL-42] Reflection warnings in users' projects Created: 10/Jun/13  Updated: 19/Aug/14  Resolved: 06/Aug/13

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.2
Fix Version/s: 0.2.4

Type: Defect Priority: Minor
Reporter: John Hume Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: reflection

Attachments: Text File 0001-Eliminate-some-reflective-calls.patch    
Patch: Code

 Description   

In a project that uses clojure.tools.nrepl.server, we see a handful of reflection warnings from nrepl.

The attached patch eliminates those.



 Comments   
Comment by John Hume [ 11/Jun/13 9:14 AM ]

I should clarify that this is an issue in 0.2.3, but JIRA doesn't know about that version yet.

Comment by Chas Emerick [ 06/Aug/13 6:02 AM ]

Patch applied @ 1c53a172a8, thanks!

There is one remaining reflection warning, only because I culled the "fix" from your patch; the reflection in that case is helping us avoid the extra code IMO.





[NREPL-45] Laziness-forcing loses *out* bindings Created: 10/Dec/13  Updated: 19/Aug/14  Resolved: 25/Feb/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.2
Fix Version/s: 0.2.4

Type: Defect Priority: Minor
Reporter: Colin Jones Assignee: Chas Emerick
Resolution: Completed Votes: 1
Labels: None

Attachments: File NREPL-45.diff     Text File nrepl-45.patch    

 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.



 Comments   
Comment by Colin Jones [ 21/Feb/14 5:19 PM ]

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.

Comment by Chas Emerick [ 25/Feb/14 4:28 AM ]

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.

Comment by Chas Emerick [ 25/Feb/14 9:36 AM ]

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).

Comment by Colin Jones [ 25/Feb/14 11:56 AM ]

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

Thanks!

Comment by Chas Emerick [ 25/Feb/14 12:04 PM ]

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





[NREPL-62] Improve Java version information Created: 22/Aug/14  Updated: 22/Aug/14  Resolved: 22/Aug/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.4
Fix Version/s: 0.2.5

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Bozhidar Batsov
Resolution: Completed Votes: 0
Labels: enhancement

Attachments: Text File 0001-Extend-Java-version-information.patch     Text File 0001-Extend-Java-version-information.patch    
Patch: Code and Test

 Description   

As discussed before - structure the Java version information similarly to nREPL and Clojure's.



 Comments   
Comment by Chas Emerick [ 22/Aug/14 8:54 AM ]

My original hesitation was largely due to a suspicion that JVM version strings are not particularly regular, especially across vendors. Perhaps that simple regex will work on all Sun/Oracle VM's, but let's keep things clean in any cases where it won't: if no match is found, let's not include the major/minor/etc slots with nil values.

Comment by Bozhidar Batsov [ 22/Aug/14 9:32 AM ]

See the updated patch - if we don't find 4 numbers in the version string we don't include any extra slots.

Comment by Chas Emerick [ 22/Aug/14 1:41 PM ]

Thanks, applied. Looks like there's often no update version included, so I tweaked things so that 3-4 slots are acceptable. e.g. http://build.clojure.org/job/tools.nrepl-test-matrix/163/CLOJURE_VERSION=1.3.0,jdk=Oracle%20JDK%201.7/console (search for java.version).

Comment by Bozhidar Batsov [ 22/Aug/14 2:53 PM ]

Yeah, turns out the first build doesn't have an update. Thanks!





[NREPL-63] Add :version-string property to the Clojure version Created: 22/Aug/14  Updated: 22/Aug/14  Resolved: 22/Aug/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.4
Fix Version/s: 0.2.5

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Bozhidar Batsov
Resolution: Completed Votes: 0
Labels: enhancement

Attachments: Text File 0001-Add-version-string-property-to-the-Clojure-version.patch    
Patch: Code and Test

 Description   

This aligns the Clojure version map with the nREPL and Java version maps (and simplifies the lives of tool writers).



 Comments   
Comment by Chas Emerick [ 22/Aug/14 9:24 PM ]

Applied as da0443d, thanks!





[NREPL-64] Add current ns to describe session's response Created: 24/Aug/14  Updated: 12/Mar/15  Resolved: 12/Mar/15

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.4
Fix Version/s: 0.2.8

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: enhancement


 Description   

In CIDER the REPL buffer gets created on receiving a describe-session response from the nREPL session that will back the REPL buffer. There's a problem with determining the default ns for the REPL buffer as many people use nREPL servers started with lein (lein repl :headless) and expect the `:init-ns` option for their project to the honored, but there's no easy way for clients to know what the default namespace is. It'd be great if this information was returned by `describe-session`.

Perhaps clients should be able to create a session with a particular default namespace to avoid the need for code like https://github.com/technomancy/leiningen/blob/554861505c23763d6583fe3b1f236a08ad02a4ca/src/leiningen/repl.clj#L103. I noticed that current the default ns is hardcoded as `user`.



 Comments   
Comment by Chas Emerick [ 25/Aug/14 8:59 PM ]

As you say, the default nREPL namespace is always user, so that's easy to know.

Leiningen's :init-ns support is implemented sanely AFAICT, given what it's aiming to accomplish. There's lots of vars in the session that one might want to customize on a per-project basis, and that tools might want to know the value of. I don't see any reason to special-case *ns*. This is one of the rare times where I'd say that a tool's best option is to use eval.

Comment by Bozhidar Batsov [ 27/Aug/14 10:09 AM ]

While I'd normally agree, the problem is that I'd have to rework cider's init in a bit awkward manner:

  • run describe session to obtain the REPL capabilities and version info
  • its callback should then do eval for ns
  • its callback should then spawn the REPL buffer

While I can certainly do this, chaining callbacks just to get some bootstrap info seems suboptimal to me. Ideally all bootstrap information
should be obtainable with a single request.
You can see the code in question here https://github.com/clojure-emacs/cider/blob/master/nrepl-client.el#L434

I can't imagine any other var value that a similar tool would like to know while bootstrapping. Obviously once you're up and running using eval is the natural solution.

Comment by Chas Emerick [ 05/Sep/14 8:05 AM ]

Not being thrilled with the idea of special-casing *ns*, I thought we might instead add a way for any middleware to contribute to the result of describe via a function in an optional descriptor slot. Each of them can be called here, and their results merged into the describe middleware's response. That function will need to take the requesting message (this will give the session middleware what it needs to provide *ns*, for example).

Happy to take a patch before I get around to this myself.

Comment by Bozhidar Batsov [ 05/Sep/14 8:25 AM ]

Sounds good to me. I guess the functions in question should be passed fully-qualified and have zero arity, right?

Comment by Chas Emerick [ 05/Sep/14 8:28 AM ]

I don't think they need to be named at all; e.g. session's :describe fn would just go here. They have to accept at least the request message as received/provided by the describe middleware.

Comment by Bozhidar Batsov [ 08/Sep/14 5:07 AM ]

Ah, I misinterpreted your previous comment. I thought you meant that we'd pass extra arguments to the describe request to get more things in the response. I think that now I understand the idea. I might take a stab at implementing this.

Comment by Chas Emerick [ 12/Mar/15 8:36 AM ]

I've committed an implementation of this to master, here. The data contributed by middlewares to the describe response is under an :aux key.

A snapshot including this change will land in the sonatype public snapshots repo shortly.





[NREPL-73] Remove reflection from NREPL-53 patch Created: 14/Feb/15  Updated: 12/Mar/15  Resolved: 12/Mar/15

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.8

Type: Defect Priority: Minor
Reporter: Gary Fredericks Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File NREPL-73-p1.patch    

 Description   

I accidentally a reflection in the accepted patch for NREPL-53.

Adding a patch here with a type hint to remove the reflection.



 Comments   
Comment by Gary Fredericks [ 14/Feb/15 3:05 PM ]

Added patch.

Comment by Chas Emerick [ 12/Mar/15 8:38 AM ]

applied to master, thanks!





[NREPL-46] nREPL crashes when required more than one time with :reload-all Created: 21/Dec/13  Updated: 02/May/15  Resolved: 12/Jan/15

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.2
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Alex Fowler Assignee: Chas Emerick
Resolution: Declined Votes: 1
Labels: bug
Environment:

Irrelevant, but the provided test case project is for CounterClockWise on Eclipse. Although it should run just fine with vanilla lein.


Attachments: Zip Archive nrepl-test.zip    

 Description   

When the namespace "clojure.tools.nrepl.server" is required more than once with :reload-all option, nREPL crashes. Accrding to current info it occures because some protocol instances get re-evaluated and are no longer the same JVM classes as they were before reload-all.

Steps to reproduce the bug in CCW:
1) Import the project into the workspace
2) Go into the core.clj
3) Click Clojure -> Load file in REPL
4) After the evaluation is complete, try evaluating (+ 1 2) in the repl. You should see no response at all.
5) Try writing something with letters, like "nrepl". The autocompletion will try and freeze and fail with the above exception.
6) Play around more to get a full hang or kill the REPL/Java to revive Eclipse.

Upon trying CCW autocompetion, the following exception occures, what might give some hint as to why:

Exception in thread "nREPL-worker-2" java.lang.IllegalArgumentException: No implementation of method: :send of protocol: #'clojure.tools.nrepl.transport/Transport found for class: clojure.tools.nrepl.middleware.pr_values$pr_values$fn$reify__1283
at clojure.core$_cache_protocol_fn.invoke(core_deftype.clj:541)
at clojure.tools.nrepl.transport$eval7291$fn_7292$G7280_7299.invoke(transport.clj:16)
at clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn_7726$fn_7739.invoke(interruptible_eval.clj:75)
at clojure.main$repl$fn__6597.invoke(main.clj:279)
at clojure.main$repl.doInvoke(main.clj:277)
at clojure.lang.RestFn.invoke(RestFn.java:1096)
at clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__7726.invoke(interruptible_eval.clj:56)
at clojure.lang.AFn.applyToHelper(AFn.java:159)
at clojure.lang.AFn.applyTo(AFn.java:151)
at clojure.core$apply.invoke(core.clj:617)
at clojure.core$with_bindings_STAR_.doInvoke(core.clj:1788)
at clojure.lang.RestFn.invoke(RestFn.java:425)
at clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invoke(interruptible_eval.clj:41)
at clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn_1345$fn_1348.invoke(interruptible_eval.clj:171)
at clojure.core$comp$fn__4154.invoke(core.clj:2330)
at clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__7760.invoke(interruptible_eval.clj:138)
at clojure.lang.AFn.run(AFn.java:24)
at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
at java.lang.Thread.run(Unknown Source)



 Comments   
Comment by Chas Emerick [ 19/Aug/14 8:56 PM ]

Confirmed. Using lein repl (though you're right that the particulars of the tooling shouldn't matter):

user=> 3
3
user=> (require '[clojure.tools.nrepl.server] :reload-all)
nil
user=> 2

user=> 1

Not sure if it's the protocol or the types that are implicated (or both), but resolving this is going to be unpleasant.

Comment by Chas Emerick [ 19/Aug/14 8:57 PM ]

Couple of questions:

  • Does the fact that this occurs matter to anyone in a meaningful way?
  • Would an acceptable solution be that reloading of the affected namespaces be no-ops?
Comment by Alex Fowler [ 25/Aug/14 9:34 AM ]

Well, in my case I have a project which is required to often reload various namespaces because they are updated in the background. If it happens that any of the namespaces refer to nrepl, the thing crashes.

The error happens due to implementation problems and is not "a correct error" from the POV of Clojure language so it is better to be fixed. However, of course, in the real world it could be rather hard to do so..

Chas, could you please explain more about the no-ops approach? Do you mean it is possible to somehow force loading nrepl just once?

Comment by Chas Emerick [ 25/Aug/14 10:20 AM ]

Yes, there are ways to prevent the re-definition of types and protocols, but I'd like to avoid doing so. It's a hack, and would be an impediment to developing nREPL itself.

Actually, you should be able to accomplish the same thing more easily in your project: rather than require nREPL namespaces in the ns form, use a top-level require form, wrapped in a suitable conditional to determine if nREPL has been loaded yet:

(when-not (resolve 'clojure.tools.nrepl/version)
  (require 'clojure.tools.nrepl))
Comment by Chas Emerick [ 12/Jan/15 8:43 AM ]

Not going to make any changes to nREPL in connection with this. :reload-all should be considered dangerous in general, IMO.

Comment by Terje Dahl [ 02/May/15 3:43 AM ]

This was very enlightening - both the fix for my mREPL-issue, as well as the warning of the danger of :reload-all. Thank you.





[NREPL-47] Two deftests with same name cause reduced test coverage Created: 23/Dec/13  Updated: 18/Apr/14  Resolved: 18/Apr/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.4

Type: Defect Priority: Trivial
Reporter: Andy Fingerhut Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: File nrepl-47-v1.diff    

 Description   

There are two deftest's with the name switch-ns in the same namespace. When this happens, the first deftest's tests are ignored, and never run. It is best if all tests are given unique names so this does not happen.

Found using a pre-release version of the Eastwood Clojure lint tool.



 Comments   
Comment by Andy Fingerhut [ 23/Dec/13 6:53 PM ]

Patch nrepl-47-v1.diff simply renames one of the deftest's named switch-ns to switch-ns-2.

Comment by Chas Emerick [ 18/Apr/14 10:06 AM ]

Thanks, fix applied.





[NREPL-61] Typo in README Created: 22/Aug/14  Updated: 22/Aug/14  Resolved: 22/Aug/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: 0.2.4
Fix Version/s: 0.2.5

Type: Defect Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Bozhidar Batsov
Resolution: Completed Votes: 0
Labels: documentation

Attachments: Text File 0001-Fix-a-typo-and-remove-some-trailing-whitespace.patch    
Patch: Code

 Description   

Just a small typo fix.



 Comments   
Comment by Chas Emerick [ 22/Aug/14 8:01 AM ]

Applied as 222c456. Thanks!





[NREPL-70] Add monroe to the list of nrepl clients Created: 04/Nov/14  Updated: 12/Jan/15  Resolved: 12/Jan/15

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.7

Type: Enhancement Priority: Trivial
Reporter: Sanel Zukan Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: documentation

Attachments: Text File NREPL-70-Add-monroe-to-the-list-of-nrepl-clients.patch    

 Description   

Hi,

Can you add monroe (https://github.com/sanel/monroe) to the list of nrepl clients in README.md, please?

Thanks!



 Comments   
Comment by Andrew Rosa [ 26/Nov/14 7:32 AM ]

Made a small patch to fix the documentation. I've also updated the CIDER description so be able to differentiate both solutions.

Comment by Bozhidar Batsov [ 11/Jan/15 4:35 AM ]

I think better descriptions would be something like "Clojure Interactive Dev Environment for Emacs" and "Basic nREPL client for Emacs". Both projects actually provide a REPL buffer. Anyways, those are just tiny details.

Comment by Chas Emerick [ 12/Jan/15 8:46 AM ]

Applied to master, thanks.





Generated at Sat Jul 04 21:52:27 CDT 2015 using JIRA 4.4#649-r158309.