<< Back to previous view

[CLJ-1221] Should repackage jsr166 and include known version with Clojure Created: 20/Jun/13  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: reducers


 Description   

Clojure 1.5 reducers work with either the JDK version of forkjoin (JDK 1.7+) or with an external jsr166 jar. This causes complexity for users and complexity in the build to deal with the two options.

jsr166 code is public domain and it is common for other projects to repackage the handful of files and ship it with the project (similar to what we do with asm). This would allow us just use a known existing version of jsr166 across all jdks and we could get rid of the custom build wrangling we introduced in Clojure 1.5.

jsr166y is compatible with JDK 1.6+ and is the version that (for example) Scala currently repackages. That's the best choice for JDK 1.6 and 1.7. In JDK 1.8, the best choice will (temporarily) be the built-in version in java.util.concurrent which tracks jsr166e but then as soon as there are updates will become jsr166e. Many fork/join fixes are ported to both y and e right now.

Some choices here for JDK 1.8:

  • go for maximal compatibility just use repackaged jsr166y regardless of JDK (simplest)
  • check for jdk version # and use java.util.concurrent instead
  • check for jdk version # and repackage jsr166e and use it instead

Not sure yet which of these is best choice right now.






[CLJ-1671] Clojure stream socket repl Created: 09/Mar/15  Updated: 10/Apr/15

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.8

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: repl

Attachments: Text File clj-1671-2.patch     Text File clj-1671-3.patch     Text File clj-1671-4.patch    
Patch: Code
Approval: Incomplete

 Description   

Programs often want to provide REPLs to users in contexts when a) network communication is desired, b) capturing stdio is difficult, or c) when more than one REPL session is desired. In addition, tools that want to support REPLs and simultaneous conversations with the host are difficult with a single stdio REPL as currently provided by Clojure.

The goal is to provide a simple streaming socket repl as part of Clojure. The socket repl should support both program-to-program communication (by exchanging data) and human-readable printing (which mirrors current behavior). The socket repl server will be started only when supplying a port to clojure.main or by explicitly starting it by calling the provided function.

Each socket connection will be given its own repl context and unique starting user namespace (user, user1, user2, etc) with separate repl stack and proper bindings. On session termination, the namespace will be removed. *in* and *out* will be bound to incoming and outgoing streams. Tools can communicate with the runtime while also providing a user repl by opening two connections to the same server.

There are two known cases where the repl interprets non-readable objects and prints them for human consumption in a non-readable way: objects with no specific print-method and when handling Throwables. Both cases (Object and Throwable) now have a print-method implementation to return a tagged-literal representation. By default the socket repl will print exceptions with the print-method data form. Optionally, the user can set a custom repl exception printer. A function that provides the current human readable exception printing will be provided.

Problems:

  1. Socket server to accept connections and serve a repl to a client
    • Runtime configuration via data
  2. Client repl sessions should be independent
    • Separate user namespace
    • Separate bindings
    • Namespace removed on client shutdown
    • Communicate solely via data for program-to-program communication
  3. Stdio has both out and err streams but socket has only single out
  4. Repls should be nestable
    • Repl within a repl binds streams appropriately
    • Means of control (exit)

Features:

  1. Printing as data
    • Of object without print-method: as #object
    • Of Throwable: as #error
  2. Start socket server from command line
    • Configure: host, port, whether to prompt, error printing
  3. Start socket server programmatically
    • Configure: host, port, whether to prompt, error printing, whether to bind err to out
    • Control: close returned socket server to stop listening
  4. Start stdio repl from command line
    • Configure: whether to prompt, error printing
  5. On socket client accept
    • Create new user namespace
    • Bind error printer according to server config
  6. In socket client repl
    • Bind new error printer function
  7. On socket client disconnect
    • Remove user namespace
    • Close socket

Impl notes: This adds a new -s option to clojure.main that will start a socket server listening on a given host:port. Each client is given a new userN namespace (starting from user1). It binds *in*, *out*, and *err*. Each client connection consumes a daemon thread named "Socket REPL Client N" (matching the user namespace). On client disconnect, the user namespace is removed and thread will die.

There are two system properties that can be used to control whether the prompt and the default error printer:

  • clojure.repl.socket.prompt (default=false) - whether to print prompts
  • clojure.repl.socket.err-printer (default=clojure.main/err->map) - function to format exceptions

The existing stdio repl behaves the same as before, but it's behavior can be influenced by two new similar system properties:

  • clojure.repl.stdio.prompt (default=true)
  • clojure.repl.socket.err-printer (default=clojure.main/err-print)

You can also start a socket repl server programmatically (shown here with all kwarg options - pick the ones you need):

(def ss 
  (clojure.main/socket-repl-server 
    :host "localhost"                   ;; default=<loopback>, accepts InetAddress or String
    :port 5555                          ;; default=0 (ephemeral)
    :use-prompt true                    ;; default=false
    :bind-err true                      ;; default=true, to bind \*err* to \*out*
    :err-printer clojure.main/err->map  ;; default=clojure.main/err->map, print Throwable to \*out*
    ))

The server socket is returned. Closing it will stop listening on the port (existing client connections will still be alive).

If you want to test with the server port, telnet makes a great client:

;; term 1:
$ java -cp target/classes -Dclojure.repl.socket.prompt=true clojure.main -s 127.0.0.1:5555

;; term 2:
$ telnet 127.0.0.1 5555
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
user1=> (+ 1 1)
2
user1=> (/ 1 0)
#error {:cause "Divide by zero",
 :via
 [{:type java.lang.ArithmeticException,
   :message "Divide by zero",
   :at [clojure.lang.Numbers divide "Numbers.java" 158]}],
 :trace
 [[clojure.lang.Numbers divide "Numbers.java" 158]
  [clojure.lang.Numbers divide "Numbers.java" 3808]
  [user1$eval1 invoke "NO_SOURCE_FILE" 1]
  [clojure.lang.Compiler eval "Compiler.java" 6784]
  [clojure.lang.Compiler eval "Compiler.java" 6747]
  [clojure.core$eval invoke "core.clj" 3078]
  [clojure.main$repl$read_eval_print__8287$fn__8290 invoke "main.clj" 265]
  [clojure.main$repl$read_eval_print__8287 invoke "main.clj" 265]
  [clojure.main$repl$fn__8296 invoke "main.clj" 283]
  [clojure.main$repl doInvoke "main.clj" 283]
  [clojure.lang.RestFn invoke "RestFn.java" 619]
  [clojure.main$socket_repl_server$fn__8342$fn__8344 invoke "main.clj" 450]
  [clojure.lang.AFn run "AFn.java" 22]
  [java.lang.Thread run "Thread.java" 724]]}
user1=> (println "hello")
hello
nil

A dynamic var clojure.main/*err-printer* is provided to customize printing of exceptions. It's bound by :err-printer if invoked programmatically or the system property if started from the command line, but it can be dynamically rebound during the session if desired:

user1=> (set! clojure.main/*err-printer* clojure.main/err-print)
#object[clojure.main$err_print 0x317bee01 "clojure.main$err_print@317bee01"]
user1=> (/ 1 0)
ArithmeticException Divide by zero  clojure.lang.Numbers.divide (Numbers.java:158)

Patch: clj-1671-4.patch



 Comments   
Comment by Timothy Baldridge [ 09/Mar/15 5:50 PM ]

Could we perhaps keep this as a contrib library? This ticket simply states "The goal is to provide a simple streaming socket repl as part of Clojure." What is the rationale for the "part of Clojure" bit?

Comment by Alex Miller [ 09/Mar/15 7:33 PM ]

We want this to be available as a Clojure.main option. It's all additive - why wouldn't you want it in the box?

Comment by Timothy Baldridge [ 09/Mar/15 10:19 PM ]

It never has really been too clear to me why some features are included in core, while others are kept in contrib. I understand that some are simply for historical reasons, but aside from that there doesn't seem to be too much of a philosophy behind it.

However it should be noted that since patches to clojure are much more guarded it's sometimes nice to have certain features in contrib, that way they can evolve with more rapidity than the one release a year that clojure has been going through.

But aside from those issues, I've found that breaking functionality into modules forces the core of a system to become more configurable. Perhaps I would like to use this repl socket feature, but pipe the data over a different communication protocol, or through a different serializer. If this feature were to be coded as a contrib library it would expose extension points that others could use to add additional functionality.

So I guess, all that to say, I'd prefer a tool I can compose rather than a pre-built solution.

Comment by Rich Hickey [ 10/Mar/15 6:25 AM ]

Please move discussions on the merits of the idea to the dev list. Comments should be about the work of resolving the ticket, approach taken by the patch, quality/perf issues etc.

Comment by Colin Jones [ 11/Mar/15 1:33 PM ]

I see that context (a) of the rationale is that network communication is desired, which sounds to me like users of this feature may want to communicate across hosts (whether in VMs or otherwise). Is that the case?

If so, it seems like specifying the address to bind to (e.g. "0.0.0.0", "::", "127.0.0.1", etc.) may become important as well as the existing port option. This way, someone who wants to communicate across hosts (or conversely, lock down access to local-only) can make that decision.

Comment by Alex Miller [ 11/Mar/15 2:07 PM ]

Colin - agreed. There are many ways to potentially customize what's in there so we need to figure out what's worth doing, both in the function and via the command line.

I think address is clearly worth having via the function and possibly in the command line too.

Comment by Kevin Downey [ 11/Mar/15 5:49 PM ]

I find the exception printing behavior really odd. for a machine you want an exception as data, but you also want some indication of if the data is an error or not, for a human you wanted a pretty printed stacktrace. making the socket repl default to printing errors this way seems to optimize for neither.

Comment by Rich Hickey [ 12/Mar/15 12:29 PM ]

Did you miss the #error tag? That indicates the data is an error. It is likely we will pprint the error data, making it not bad for both purposes

Comment by Alex Miller [ 13/Mar/15 11:29 AM ]

New -4 patch changes:

  • clojure.core/throwable-as-map now public and named clojure.core/Throwable->map
  • catch and ignore SocketException without printing in socket server repl (for client disconnect)
  • functions to print as message and as data are now: clojure.main/err-print and clojure.main/err->map. All defaults and docs updated.
Comment by David Nolen [ 18/Mar/15 12:44 PM ]

Is there any reason to not allow supplying :eval in addition to :use-prompt? In the case of projects like ClojureCLR + Unity eval generally must happen on the main thread. With :eval as something which can be configured, REPL sessions can queue forms to be eval'ed with the needed context (current ns etc.) to the main thread.

Comment by Kevin Downey [ 20/Mar/15 2:12 PM ]

I did see the #error tag, but throwables print with that tag regardless of if they are actually thrown or if they are just the value returned from a function. Admittedly returning exceptions as values is not something generally done, but the jvm does distinguish between a return value and a thrown exception. Having a repl that doesn't distinguish between the two strikes me as an odd design. The repl you get from clojure.main currently prints the message from a thrown uncaught throwable, and on master prints with #error if you have a throwable value, so it distinguishes between an uncaught thrown throwable and a throwable value. That obviously isn't great for tooling because you don't get a good data representation in the uncaught case.

It looks like the most recent patch does pretty print uncaught throwables, which is helpful for humans to distinguish between a returned value and an uncaught throwable.

Comment by Kevin Downey [ 25/Mar/15 1:10 PM ]

alex: saying this is all additive, when it has driven changes to how things are printed, using the global print-method, rings false to me

Comment by Sam Ritchie [ 25/Mar/15 1:15 PM ]

This seems like a pretty big last minute addition for 1.7. What's the rationale for adding it here vs deferring to 1.8, or trying it out as a contrib first?

Comment by Alex Miller [ 25/Mar/15 2:13 PM ]

Kevin: changing the fallthrough printing for things that are unreadable to be readable should be useful regardless of the socket repl. It shouldn't be a change for existing programs (unless they're relying on the toString of objects without print formats).

Sam: Rich wants it in the box as a substrate for tools.

Comment by Alex Miller [ 26/Mar/15 10:03 AM ]

Marking incomplete, pending at least the repl exit question.





[CLJ-1669] Move LazyTransformer to an iterator strategy, extend eduction capabilities Created: 04/Mar/15  Updated: 10/Apr/15

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: transducers

Attachments: Text File clj-1669-2.patch     Text File clj-1669-3.patch     Text File clj-1669-4.patch     Text File clj-1669-5.patch     Text File clj-1669-6.patch     Text File clj-1669.patch    
Patch: Code and Test
Approval: Ok

 Description   
  • LazyTransformer does a lot of work to be a seq. Instead, switch to creating a transforming iterator.
  • Change sequence to wrap iterator-seq around the transforming iterator.
  • Change the iterator-seq implementation to be chunked. IteratorSeq will no longer be used but is left in case of regressions for now.
  • Change Eduction to provide iteration directly via the transforming iterator.
  • Extend eduction to support multiple xforms.

Performance:

(use 'criterium.core)
(def s (range 1000))
(def v (vec s))
(def s50 (range 50))
(def v50 (vec s50))

expr master s master v 1669-3 s 1669-3 v 1669-6 s 1669-6 v
non-chunking transform            
(into [] (->> s (interpose 5) (partition-all 2))) 466 us 459 us 525 us 508 us 476 us 501 us
(into [] (->> s (eduction (interpose 5) (partition-all 2)))) * 113 us 112 us 117 us 122 us 108 us 108 us
1 chunking transform            
(into [] (map inc s)) 28 us 31 us 30 us 31 us 30 us 31 us
(into [] (map inc) s) 17 us 19 us 19 us 20 us 19 us 17 us
(into [] (sequence (map inc) s)) 58 us 46 us 142 us 148 us 94 us 67 us
(into [] (eduction (map inc) s)) 21 us 20 us 25 us 21 us 23 us 21 us
(doall (map inc (eduction (map inc) s))) 219 us 208 us 204 us 191 us 117 us 97 us
2 chunking transforms        
(into [] (map inc (map inc s))) 49 us 50 us 50 us 50 us 54 us 55 us
(into [] (comp (map inc) (map inc)) s) 23 us 23 us 28 us 23 us 23 us 23 us
(into [] (sequence (comp (map inc) (map inc)) s)) 73 us 58 us 144 us 135 us 104 us 82 us
(into [] (eduction (map inc) (map inc) s)) * 54 us 51 us 54 us 29 us 55 us 30 us
(doall (map inc (eduction (map inc) (map inc) s))) * 230 us 213 us 213 us 196 us 124 us 104 us
expand transform            
(into [] (mapcat range (map inc s50))) 83 us 81 us 80 us 84 us 71 us 72 us
(into [] (sequence (comp (map inc) (mapcat range)) s50)) 122 us 117 us 256 us 254 us 161 us 156 us
(into [] (eduction (map inc) (mapcat range) s50)) * 78 us 79 us 80 us 82 us 60 us 61 us
materialized eduction            
(sort (eduction (map inc) s)) ERR ERR 120 us 84 us 106 us 89 us
(->> s (filter odd?) (map str) (sort-by last)) 1.13 ms 1.21 ms 1.19 ms 1.20 ms 1.19 ms 1.20 ms
(->> s (eduction (filter odd?) (map str)) (sort-by last)) ERR ERR 1.18 ms 1.17 ms 1.22 ms 1.23 ms
  • used comp to combine xforms as eduction only took one in the before case

Patch: clj-1669-6.patch

Screened by:



 Comments   
Comment by Michael Blume [ 05/Mar/15 3:52 PM ]

Nice, I like the direction on this.

CLJ-1515 currently breaks this patch (LongRange cannot be converted to Iterable), but I imagine that'll get better when it absorbs the changes from CLJ-1603

Comment by Alex Miller [ 06/Mar/15 8:11 AM ]

Yeah. colls should be mapped through RT.iter() to catch more cases.

Comment by Alex Miller [ 06/Mar/15 9:52 AM ]

To do:

  • remove Seqable from Eduction
  • support Iterable in RT.toArray()
  • more eduction pipeline tests that require realization at end
Comment by Alex Miller [ 06/Mar/15 1:00 PM ]

Perf numbers show pretty worse results from sequence, will dig in further.

Comment by Alex Miller [ 13/Mar/15 7:41 AM ]

For the s timings, we've actually introduced more steps into the stack:

OLD reduce with s:

LazyTransformer
   seq (range) - every transformation is another layer here

NEW reduce with s:

IteratorSeq 
  TransformingIterator (handles N transformations in 1 step)
    SeqIterator
      seq (range)
Comment by Alex Miller [ 20/Mar/15 10:08 AM ]

Look at perf for:

  • ->> eduction transformation
  • transformation comparison that doesn't support chunking
  • more into vector iteration case
Comment by Alex Miller [ 21/Mar/15 8:45 AM ]

The -5 patch is same -3 except all uses of IteratorSeq have been replaced with a ChunkedCons that is effectively a chunked version of the old IteratorSeq. While no one calls it, I left IteratorSeq in the code base in case of regression.

Generally, the chunked iterator seq reduces the cost in a number of the worst cases and also is a clear benefit in making seqs over a result of eduction or sequence faster to traverse (as they are now chunked).

I think the one potential issue is that seqs over iterators are now chunked when they were not before which could change programs that expect their stateful iterator to be traversed one at a time. This change could be isolated to just to sequence and seq-iterator and mitigated by not changing RT.seqFrom() and seq-iterator to use the new chunking behavior only in sequence and/or with a new chunked-iterator-seq to make it more explicit. The sequence over xf is new so no possible regression there, everything else would just be opt-in.

Comment by Rich Hickey [ 27/Mar/15 9:49 AM ]

push as is but leave unresolved, for perf tweaks

Comment by Alex Miller [ 27/Mar/15 10:15 AM ]

clj-1669-6 is identical to clj-1669-5 but removes two commented out debugging lines that were inadvertently included.





[CLJ-1703] Pretty print #error data Created: 14/Apr/15  Updated: 17/Apr/15

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: print

Attachments: Text File clj-1703-2.patch     Text File clj-1703.patch    
Patch: Code
Approval: Screened

 Description   

Some of the work we were doing re socket repls got pushed out but it would still be nice to expose the pretty printed #error formatting as the current version is very hard to read.

1.7.0-beta1:

user=> *99

CompilerException java.lang.RuntimeException: Unable to resolve symbol: *99 in this context, compiling:(/private/var/folders/7r/_1fj0f517rgcxwx79mn79mfc0000gn/T/form-init8696775124159270468.clj:1:1263)
user=> (prn *e)
#error{:cause "Unable to resolve symbol: *99 in this context", :via [{:type clojure.lang.Compiler$CompilerException, :message "java.lang.RuntimeException: Unable to resolve symbol: *99 in this context, compiling:(/private/var/folders/7r/_1fj0f517rgcxwx79mn79mfc0000gn/T/form-init8696775124159270468.clj:1:1263)", :at [clojure.lang.Compiler analyze "Compiler.java" 6543]} {:type java.lang.RuntimeException, :message "Unable to resolve symbol: *99 in this context", :at [clojure.lang.Util runtimeException "Util.java" 221]}], :trace [[clojure.lang.Util runtimeException "Util.java" 221] [clojure.lang.Compiler resolveIn "Compiler.java" 7029] [clojure.lang.Compiler resolve "Compiler.java" 6973] [clojure.lang.Compiler analyzeSymbol "Compiler.java" 6934] [clojure.lang.Compiler analyze "Compiler.java" 6506] [clojure.lang.Compiler analyze "Compiler.java" 6485] [clojure.lang.Compiler eval "Compiler.java" 6796] [clojure.lang.Compiler eval "Compiler.java" 6755] [clojure.core$eval invoke "core.clj" 3082] [clojure.main$repl$read_eval_print__7057$fn__7060 invoke "main.clj" 240] [clojure.main$repl$read_eval_print__7057 invoke "main.clj" 240] [clojure.main$repl$fn__7066 invoke "main.clj" 258] [clojure.main$repl doInvoke "main.clj" 258] [clojure.lang.RestFn invoke "RestFn.java" 1523] [clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__599 invoke "interruptible_eval.clj" 53] [clojure.lang.AFn applyToHelper "AFn.java" 152] [clojure.lang.AFn applyTo "AFn.java" 144] [clojure.core$apply invoke "core.clj" 628] [clojure.core$with_bindings_STAR_ doInvoke "core.clj" 1866] [clojure.lang.RestFn invoke "RestFn.java" 425] [clojure.tools.nrepl.middleware.interruptible_eval$evaluate invoke "interruptible_eval.clj" 51] [clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn__641$fn__644 invoke "interruptible_eval.clj" 183] [clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__634 invoke "interruptible_eval.clj" 152] [clojure.lang.AFn run "AFn.java" 22] [java.util.concurrent.ThreadPoolExecutor runWorker "ThreadPoolExecutor.java" 1142] [java.util.concurrent.ThreadPoolExecutor$Worker run "ThreadPoolExecutor.java" 617] [java.lang.Thread run "Thread.java" 744]]}

Approach: Do some minimal formatting of the #error data, should be pretty close to (pprint (Throwable->map *e)).

w/patch:

user=> (prn *e)
#error {
 :cause "Unable to resolve symbol: *99 in this context"
 :via
 [{:type clojure.lang.Compiler$CompilerException
   :message "java.lang.RuntimeException: Unable to resolve symbol: *99 in this context, compiling:(NO_SOURCE_PATH:0:0)"
   :at [clojure.lang.Compiler analyze "Compiler.java" 6543]}
  {:type java.lang.RuntimeException
   :message "Unable to resolve symbol: *99 in this context"
   :at [clojure.lang.Util runtimeException "Util.java" 221]}]
 :trace
 [[clojure.lang.Util runtimeException "Util.java" 221]
  [clojure.lang.Compiler resolveIn "Compiler.java" 7029]
  [clojure.lang.Compiler resolve "Compiler.java" 6973]
  [clojure.lang.Compiler analyzeSymbol "Compiler.java" 6934]
  [clojure.lang.Compiler analyze "Compiler.java" 6506]
  [clojure.lang.Compiler analyze "Compiler.java" 6485]
  [clojure.lang.Compiler eval "Compiler.java" 6796]
  [clojure.lang.Compiler eval "Compiler.java" 6755]
  [clojure.core$eval invoke "core.clj" 3079]
  [clojure.main$repl$read_eval_print__7093$fn__7096 invoke "main.clj" 240]
  [clojure.main$repl$read_eval_print__7093 invoke "main.clj" 240]
  [clojure.main$repl$fn__7102 invoke "main.clj" 258]
  [clojure.main$repl doInvoke "main.clj" 258]
  [clojure.lang.RestFn invoke "RestFn.java" 421]
  [clojure.main$repl_opt invoke "main.clj" 324]
  [clojure.main$main doInvoke "main.clj" 422]
  [clojure.lang.RestFn invoke "RestFn.java" 397]
  [clojure.lang.Var invoke "Var.java" 375]
  [clojure.lang.AFn applyToHelper "AFn.java" 152]
  [clojure.lang.Var applyTo "Var.java" 700]
  [clojure.main main "main.java" 37]]}

Patch: clj-1703-2.patch



 Comments   
Comment by Rich Hickey [ 17/Apr/15 9:48 AM ]

Can we please put the kv pairs of via each on their own line?





[CLJ-124] GC Issue 120: Determine mechanism for controlling automatic shutdown of Agents, with a default policy and mechanism for changing that policy as needed Created: 17/Jun/09  Updated: 23/Aug/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Chas Emerick Assignee: Alex Miller
Resolution: Unresolved Votes: 7
Labels: agents

Attachments: Text File clj-124-daemonthreads-v1.patch     Text File clj-124-v1.patch    
Patch: Code
Approval: Vetted
Waiting On: Rich Hickey

 Description   

The original description when this ticket was vetted is below, starting with "Reported by cemer...@snowtide.com, June 01, 2009". This prefix attempts to summarize the issue and discussion.

Description:

Several Clojure functions involving agents and futures, such as future, pmap, clojure.java.shell/sh, and a few others, create non-daemon threads in the JVM in an ExecutorService called soloExecutor created via Executors#newCachedThreadPool. The javadocs for this method here http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executors.html#newCachedThreadPool%28%29 say "Threads that have not been used for sixty seconds are terminated and removed from the cache." This causes a 60-second wait after a Clojure program is done before the JVM process exits. Questions about this confusing behavior come up a couple of times per year on the Clojure Google group. Search for "shutdown-agents" to find most of these occurrences, since calling (shutdown-agents) at the end of one's program typically eliminates this 60-second wait.

Example:

% java -cp clojure.jar clojure.main -e "(println 1)"
1
[ this case exits quickly ]

% java -cp clojure.jar clojure.main -e "(println @(future 1))"
1
[ 60-second pause before process exits, at least on many platforms and JVMs ]

Summary of comments before July 2014:

Most of the comments on this ticket on or before August 23, 2010 were likely spread out in time before being imported from the older ticket tracking system into JIRA. Most of them refer to an older suggested patch that is not in JIRA, and compilation problems it had with JDK 1.5, which is no longer supported by Clojure 1.6.0. I think these comments can be safely ignored now.

Alex Miller blogged about this and related issues here: http://tech.puredanger.com/2010/06/08/clojure-agent-thread-pools/

Since then, two of the suggestions Alex raised have been addressed. One by CLJ-378 and one by the addition of set-agent-send-executor! and similar functions to Clojure 1.5.0: https://github.com/clojure/clojure/blob/master/changes.md#23-clojurecoreset-agent-send-executor-set-agent-send-off-executor-and-send-via

One remaining issue is the topic of this ticket, which is how best to avoid this 60-second pause.

Approach #1: automatically shut down agents

One method is mentioned in Chas Emerick's original description below, suggested by Rich Hickey, but perhaps long enough ago he may no longer endorse it: Create a Var *auto-shutdown-agents* that when true (the default value), clojure.lang.Agent shutdown() is called after the clojure.main entry point. This removes the surprising wait for common methods of starting Clojure, while allowing expert users to change that value to false if desired.

Approach #2: create daemon threads by default

Another method mentioned by several people in the comments is to change the threads created in agent thread pools to daemon threads by default, and perhaps to deprecate shutdown-agents or modify it to be less dangerous. That approach is discussed a bit more in Alex's blog post linked above, and in a comment from Alexander Taggart on July 11, 2011 below.

Approach #3:

The only other comment before 2014 that is not elaborated in this summary is shoover's suggestion: There are already well-defined and intuitive ways to block on agents and futures. Why not deprecate shutdown-agents and force users to call await and deref if they really want to block? In the pmap situation one would have to evaluate the pmap form.

Approach #4: Create a cached thread pool with a timeout much lower than 60 seconds

This could be done by using one of the ThreadPoolExecutor constructors with a keepAliveTime parameter of the desired time.

Patch: clj-124-v1.patch clj-124-daemonthreads-v1.patch

At most one of these patches should be considered, depending upon the desired approach to take.

Patch clj-124-v1.patch implements appproach #1 using *auto-shutdown-agents*. See the Jul 31 2014 comment when this patch was added for some additional details.

Patch clj-124-daemonthreads-v1.patch implements approach #2 and is straightforward.

Reported by cemer...@snowtide.com, Jun 01, 2009

There has been intermittent chatter over the past months from a couple of
people on the group (e.g.
http://groups.google.com/group/clojure/browse_thread/thread/409054e3542adc1f)
and in #clojure about some clojure scripts hanging, either for a constant
time (usually reported as a minute or so with no CPU util) or seemingly
forever (or until someone kills the process).

I just hit a similar situation in our compilation process, which invokes
clojure.lang.Compile from ant.  The build process for this particular
project had taken 15 second or so, but after adding a couple of pmap calls,
that build time jumped to ~1:15, with roughly zero CPU utilization over the
course of that last minute.

Adding a call to Agent.shutdown() in the finally block in
clojure.lang.Compile/main resolved the problem; a patch including this
change is attached.  I wouldn't suspect anyone would have any issues with
such a change.

-----
In general, it doesn't seem like everyone should keep tripping over this
problem in different directions.  It's a very difficult thing to debug if
you're not attuned to how clojure's concurrency primitives work under the
hood, and I would bet that newer users would be particularly affected.

After discussion in #clojure, rhickey suggested adding a
*auto-shutdown-agents* var, which:

- if true when exiting one of the main entry points (clojure.main, or the
legacy script/repl entry points), Agent.shutdown() would be called,
allowing for the clean exit of the application

- would be bound by default to true

- could be easily set to false for anyone with an advanced use-case that
requires agents to remain active after the main thread of the application
exits.

This would obviously not help anyone initializing clojure from a different
entry point, but this may represent the best compromise between
least-surprise and maximal functionality for advanced users.

------

In addition to the above, it perhaps might be worthwhile to change the
keepalive values used to create the Threadpools used by c.l.Actor's
Executors.  Currently, Actor uses a default thread pool executor, which
results in a 60s keepalive.  Lowering this to something much smaller (1s?
5s?) would additionally minimize the impact of Agent's threadpools on Java
applications that embed clojure directly (and would therefore not benefit
from *auto-shutdown-agents* as currently conceived, leading to puzzling
'hanging' behaviour).  I'm not in a position to determine what impact this
would have on performance due to thread churn, but it would at least
minimize what would be perceived as undesirable behaviour by users that are
less familiar with the implementation details of Agent and code that
depends on it.

Comment 1  by cemer...@snowtide.com, Jun 01, 2009

Just FYI, I'd be happy to provide patches for either of the suggestions mentioned
above...


 Comments   
Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/124
Attachments:
compile-agent-shutdown.patch - https://www.assembla.com/spaces/clojure/documents/a56S2ow4ur3O2PeJe5afGb/download/a56S2ow4ur3O2PeJe5afGb
124-compilation.diff - https://www.assembla.com/spaces/clojure/documents/aqn0IGxZSr3RUGeJe5aVNr/download/aqn0IGxZSr3RUGeJe5aVNr

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

oranenj said: [file:a56S2ow4ur3O2PeJe5afGb]

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

cemerick said: (In [[r:fa3d24973fc415b35ae6ec8d84b61ace76bd4133]]) Add a call to Agent.shutdown() at the end of clojure.lang.Compile/main Refs #124

Signed-off-by: Chouser <chouser@n01se.net>

Branch: master

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: I'm closing this ticket to because the attached patch solves a specific problem. I agree that the idea of an auto-shutdown-agents var sounds like a positive compromise. If Rich wants a ticket to track that issue, I think it'd be best to open a new ticket (and perhaps mention this one there) rather than use this ticket to track further changes.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

scgilardi said: With both Java 5 and Java 6 on Mac OS X 10.5 Leopard I'm getting an error when compiling with this change present.

Java 1.5.0_19
Java 1.6.0_13

For example, when building clojure using "ant" from within my clone of the clojure repo:

[java] java.security.AccessControlException: access denied (java.lang.RuntimePermission modifyThread)
[java] at java.security.AccessControlContext.checkPermission(AccessControlContext.java:264)
[java] at java.security.AccessController.checkPermission(AccessController.java:427)
[java] at java.util.concurrent.ThreadPoolExecutor.shutdown(ThreadPoolExecutor.java:894)
[java] at clojure.lang.Agent.shutdown(Agent.java:34)
[java] at clojure.lang.Compile.main(Compile.java:71)

I reproduced this on two Mac OS X 10.5 machines. I'm not aware of having any enhanced security policies along these lines on my machines. The compile goes fine for me with Java 1.6.0_0 on an Ubuntu box.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: I had only tested it on my ubuntu box – looks like that was openjdk 1.6.0_0. I'll test again with sun-java5 and sun-java6.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: 1.6.0_13 worked fine for me on ubuntu, but 1.5.0_18 generated an the exception Steve pasted. Any suggestions? Should this patch be backed out until someone has a fix?

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

achimpassen said: [file:aqn0IGxZSr3RUGeJe5aVNr]

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: With Achim's patch, clojure compiles for me on ubuntu using java 1.5.0_18 from sun, and still works on 1.6.0_13 sun and 1.6.0_0 openjdk. I don't know anything about ant or the security error, but this is looking good to me.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

achimpassen said: It works for me on 1.6.0_13 and 1.5.0_19 (32 and 64 bit) on OS X 10.5.7.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: (In [[r:895b39dabc17b3fd766fdbac3b0757edb0d4b60d]]) Rev fa3d2497 causes compile to fail on some VMs – back it out. Refs #124

Branch: master

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

mikehinchey said: I got the same compile error on both 1.5.0_11 and 1.6.0_14 on Windows. Achim's patch fixes both.

See the note for "permissions" on http://ant.apache.org/manual/CoreTasks/java.html . I assume ThreadPoolExecutor.shutdown is the problem, it would shutdown the main Ant thread, so Ant disallows that. Forking avoids the permissions limitation.

In addition, since the build error still resulted in "BUILD SUCCESSFUL", I think failonerror="true" should also be added to the java call so the build would totally fail for such an error.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: I don't know if the <java fork=true> patch is a good idea or not, or if there's a better way to solve the original problem.

Chas, I'm kicking back to you, but I guess if you don't want it you can reassign to "nobody".

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

shoover said: I'd like to suggest an alternate approach. There are already well-defined and intuitive ways to block on agents and futures. Why not deprecate shutdown-agents and force users to call await and deref if they really want to block? In the pmap situation one would have to evaluate the pmap form.

The System.exit problem goes away if you configure the threadpools to use daemon threads (call new ThreadPoolExecutor and pass a thread factory that creates threads and sets daemon to true). That way the user has an explicit means of blocking and System.exit won't hang.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

alexdmiller said: I blogged about these issues at:
http://tech.puredanger.com/2010/06/08/clojure-agent-thread-pools/

I think that:

  • agent thread pool threads should be named (see ticket #378)
  • agent thread pools must be daemon threads by default
  • having ways to specify an customized executor pool for an agent send/send-off is essential to customize threading behavior
  • (shutdown-agents) should be either deprecated or made less dangerous
Comment by Alexander Taggart [ 11/Jul/11 9:33 PM ]

Rich, what is the intention behind using non-daemon threads in the agent pools?

If it is because daemon threads could terminate before their work is complete, would it be acceptable to add a shutdown hook to ensure against such premature termination? Such a shutdown hook could call Agent.shutdown(), then awaitTermination() on the pools.

Comment by Christopher Redinger [ 27/Nov/12 3:47 PM ]

Moving this ticket out of approval "OK" status, and dropping the priority. These were Assembla import defaults.

Also, Chas gets to be the Reporter now.

Comment by Chas Emerick [ 27/Nov/12 5:56 PM ]

Heh, blast from the past.

The comment import appears to have set their timestamps to the date of the import, so the conversation is pretty hard to follow, and obviously doesn't benefit from the intervening years of experience. In addition, there have been plenty of changes to agents, including some recent enhancements that address some of the pain points that Alex Miller mentioned above.

I propose closing this as 'invalid' or whatever, and opening one or more new issues to track whatever issues still persist (presumably based on fresh ML discussion, etc).

Comment by Andy Fingerhut [ 27/Nov/12 6:11 PM ]

Rereading the original description of this ticket, without reading all of the comments that follow, that description is still right on target for the behavior of latest Clojure master today.

People send messages to the Clojure Google group every couple of months hitting this issue, and one even filed CLJ-959 because of hitting it. I have updated the examples on ClojureDocs.org for future, and also for pmap and clojure.java.shell/sh which use future in their implementations, to warn people about this and explain that they should call (shutdown-agents), but making it unnecessary to call shutdown-agents would be even better, at least as the default behavior. It sounds fine to me to provide a way for experts on thread behavior to change that default behavior if they need to.

Comment by Andy Fingerhut [ 31/Jul/14 6:39 PM ]

Patch clj-124-v1.patch dated Jul 31 2014 implements the approach of calling clojure.lang.Agent#shutdown when the new Var *auto-shutdown-agents* is true, which is its default value.

I don't see any benefit to making this Var dynamic. Unless I am missing something, only the root binding value is visible after clojure.main/main returns, not any binding that would be pushed on top of that if it were dynamic. It seems to require alter-var-root to change it to false in a way that this patch would avoid calling clojure.lang.Agent#shutdown.

This patch only adds the shutdown call to clojure.main#main, but can easily be added to the legacy_repl and legacy_script methods if desired.

Comment by Andy Fingerhut [ 23/Aug/14 11:49 AM ]

Patch clj-124-daemonthreads-v1.patch dated Aug 23 2014 simply modifies the ThreadFactory so that every thread created in an agent thread pool is a daemon thread.





Generated at Tue Apr 21 08:22:37 CDT 2015 using JIRA 4.4#649-r158309.