<< Back to previous view

[NREPL-29] Provide a mechanism for overriding an operation Created: 09/Sep/12  Updated: 18/Apr/14

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

Type: Enhancement Priority: Major
Reporter: Hugo Duncan Assignee: Chas Emerick
Resolution: Unresolved Votes: 0
Labels: None


 Description   

When specifying middleware, it would be much easier for the user to be able to override a default middleware without having to specify a handler.

For example, if there is a default middleware providing the "complete" operation, the user should be able to just specify their preferred completion middleware, without having to specify all middleware as a handler.

One way to do this might be to check metadata for the provided operations of the specified metadata, and ensure that either the default middleware for that operation is removed, or that the specified middleware takes precendence (which may be simpler when a middleware provides multiple operations).



 Comments   
Comment by Chas Emerick [ 14/Sep/12 7:44 AM ]

Agreed.

Just making sure that the order in which additional middlewares are provided is taken as a default stack order will suffice for most use cases. Transforming middlewares (either in full or in part) would need another metadata slot, :replace perhaps, though it seems like that would be much more difficult to get right.





[NREPL-36] Too many DynamicClassLoaders created Created: 10/Dec/12  Updated: 18/Apr/14

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

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


 Description   

Not sure whether this ticket belongs here or in the Clojure-proper JIRA, so feel free to close if this is an inappropriate location. clojure.main/repl creates a new DynamicClassLoader on every execution, so it looks like the stack of classloaders grows without bounds. Seems a bit similar to http://dev.clojure.org/jira/browse/NREPL-31 in that clojure.main/repl has another assumption about when clojure.main/repl will run.

See https://groups.google.com/forum/?fromgroups=#!topic/clojure/firG9zTVecU%5B1-25%5D for the original report.



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

Yes, this should be fixed upstream; a new DynamicClassLoader should only be set as the thread-context classloader if one is not already in place.

My first impulse upon seeing this was that this was "last straw" territory w.r.t. using clojure.main/repl (as recorded in the thread linked above), but the work necessary to stop depending upon it would be considerable (not because repl does much, but because interruptible-eval/evaluate is structured to cater to it). Some years on, and it's clear that this fundamentally a minor problem (insofar as hardly anyone has complained AFAIK), so a fix that requires e.g. Clojure 1.7.0 would be fine.





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

Status: In Progress
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: Unresolved 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.





[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-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-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-45] Laziness-forcing loses *out* bindings Created: 10/Dec/13  Updated: 12/Apr/14  Resolved: 25/Feb/14

Status: Resolved
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.





Generated at Sat Apr 19 07:38:53 CDT 2014 using JIRA 4.4#649-r158309.