<< Back to previous view

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





Generated at Thu Aug 21 07:18:19 CDT 2014 using JIRA 4.4#649-r158309.