<< Back to previous view

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





Generated at Thu Nov 27 22:28:00 CST 2014 using JIRA 4.4#649-r158309.