Clojure

GC Issue 120: Determine mechanism for controlling automatic shutdown of Agents, with a default policy and mechanism for changing that policy as needed

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
    None
  • Approval:
    Vetted

Description

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

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Assembla Importer added a comment -

oranenj said: [file:a56S2ow4ur3O2PeJe5afGb]

Show
Assembla Importer added a comment - oranenj said: [file:a56S2ow4ur3O2PeJe5afGb]
Hide
Permalink
Assembla Importer added a comment -

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)

Show
Assembla Importer added a comment - 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)
Hide
Permalink
Assembla Importer added a comment -

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

Show
Assembla Importer added a comment - 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
Hide
Permalink
Assembla Importer added a comment -

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.

Show
Assembla Importer added a comment - 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.
Hide
Permalink
Assembla Importer added a comment -

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.

Show
Assembla Importer added a comment - 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.
Hide
Permalink
Assembla Importer added a comment -

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.

Show
Assembla Importer added a comment - 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.
Hide
Permalink
Assembla Importer added a comment -

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?

Show
Assembla Importer added a comment - 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?
Hide
Permalink
Assembla Importer added a comment -

achimpassen said: [file:aqn0IGxZSr3RUGeJe5aVNr]

Show
Assembla Importer added a comment - achimpassen said: [file:aqn0IGxZSr3RUGeJe5aVNr]
Hide
Permalink
Assembla Importer added a comment -

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.

Show
Assembla Importer added a comment - 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.
Hide
Permalink
Assembla Importer added a comment -

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.

Show
Assembla Importer added a comment - 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.
Hide
Permalink
Assembla Importer added a comment -

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

Branch: master

Show
Assembla Importer added a comment - chouser@n01se.net said: (In [[r:895b39dabc17b3fd766fdbac3b0757edb0d4b60d]]) Rev fa3d2497 causes compile to fail on some VMs – back it out. Refs #124 Branch: master
Hide
Permalink
Assembla Importer added a comment -

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.

Show
Assembla Importer added a comment - 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.
Hide
Permalink
Assembla Importer added a comment -

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

Show
Assembla Importer added a comment - 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".
Hide
Permalink
Assembla Importer added a comment -

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

Show
Assembla Importer added a comment - richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)
Hide
Permalink
Assembla Importer added a comment -

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.

Show
Assembla Importer added a comment - 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.
Hide
Permalink
Assembla Importer added a comment -

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
Show
Assembla Importer added a comment - 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
Hide
Permalink
Alexander Taggart added a comment -

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.

Show
Alexander Taggart added a comment - 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.
Hide
Permalink
Christopher Redinger added a comment -

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.

Show
Christopher Redinger added a comment - 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.
Hide
Permalink
Chas Emerick added a comment -

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

Show
Chas Emerick added a comment - 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).
Hide
Permalink
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.

People

Vote (3)
Watch (5)

Dates

  • Created:
    Updated:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.