Clojure

promote interrupt handling to clojure.repl

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Blocker Blocker
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.3
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code
  • Approval:
    Ok

Description

The functions start-handling-break and add-break-thread! in clojure.contrib.repl-utils are regularly needed by tools such as the Clojure Debugging Toolkit. I propose we promote them to clojure.repl, eliminating what for many projects is their sole dependency on old contrib.

Issues to consider before OKing:

  • the usage of sun.misc classes appears to be the only game in town. For comparison, JRuby and Groovy already offer access to these classes, Scala appears not to.
  • the :need-init code in start-handling-break is too cute, but ain't broke, so won't change it.
  • do we need to add code to clean up the weak references? If so:
    • call each time a thread is added?
    • switch to ConcurrentHashMap to match code already in Clojure?

Activity

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

Converted from http://www.assembla.com/spaces/clojure/tickets/460

Show
Assembla Importer added a comment - Converted from http://www.assembla.com/spaces/clojure/tickets/460
Hide
Permalink
Assembla Importer added a comment -

richhickey said: Is there a patch to look at?

Show
Assembla Importer added a comment - richhickey said: Is there a patch to look at?
Hide
Permalink
Assembla Importer added a comment -

chouser@n01se.net said: 'add-break-thread!' is too compound, presumably to make it easier to give instructions in IRC. No reason it couldn't be simplified, making the instructions to set up two steps, one to register a thread to break, another to start catching the Ctrl-C. I'll prepare a patch and docs for this.

Show
Assembla Importer added a comment - chouser@n01se.net said: 'add-break-thread!' is too compound, presumably to make it easier to give instructions in IRC. No reason it couldn't be simplified, making the instructions to set up two steps, one to register a thread to break, another to start catching the Ctrl-C. I'll prepare a patch and docs for this.
Hide
Permalink
Chouser added a comment -

This patch avoids the issues of cleaning up weak references by only directly supporting one thread to stop at a time. I think this is a sensible compromise as you'll generally only have one main REPL thread at a time. In a more complex situation, stopping all registered threads isn't obviously the right thing to do. set-break-handler! accepts a function that can do arbitrarily complex things as needed.

This patch also avoids the cuteness of :need-init by simply relying on the calling process to only call set-break-handler! once, which any REPL-startup script should be able to easily comply with. It appears that repeated calls to set-break-handler! will just overwrite the old handler with the new one – if their the same (such as the default handler) then no harm done. If they're not the same, refusing to overwrite the old one is not obviously better than just taking the new handler.

Show
Chouser added a comment - This patch avoids the issues of cleaning up weak references by only directly supporting one thread to stop at a time. I think this is a sensible compromise as you'll generally only have one main REPL thread at a time. In a more complex situation, stopping all registered threads isn't obviously the right thing to do. set-break-handler! accepts a function that can do arbitrarily complex things as needed. This patch also avoids the cuteness of :need-init by simply relying on the calling process to only call set-break-handler! once, which any REPL-startup script should be able to easily comply with. It appears that repeated calls to set-break-handler! will just overwrite the old handler with the new one – if their the same (such as the default handler) then no harm done. If they're not the same, refusing to overwrite the old one is not obviously better than just taking the new handler.
Hide
Permalink
Rich Hickey added a comment -

I think the default should throw a derivee of Error, not Exception, as there are many Exception-eating paths in arbitrary code.

Show
Rich Hickey added a comment - I think the default should throw a derivee of Error, not Exception, as there are many Exception-eating paths in arbitrary code.
Hide
Permalink
Chouser added a comment -

Ah, figured out how to set to "Test". I assume this is ok to do, even though I'm not "Core team"?

Show
Chouser added a comment - Ah, figured out how to set to "Test". I assume this is ok to do, even though I'm not "Core team"?
Hide
Permalink
Chouser added a comment -

Bah, sorry, missed Rich's comment. Back to Approval: None. Will fix the patch.

Show
Chouser added a comment - Bah, sorry, missed Rich's comment. Back to Approval: None. Will fix the patch.
Hide
Permalink
Chouser added a comment -

The two reasonable choices of existing Errors to throw are Error and ThreadDeath. ThreadDeath is the default for thread .stop, but doesn't provide a mechanism for setting the message. I think it's more important to specify the exact cause of the exception, that a SIGINT was caught, than to use a possibly more accurate exception type, so the new patch throws a plain Error.

Show
Chouser added a comment - The two reasonable choices of existing Errors to throw are Error and ThreadDeath. ThreadDeath is the default for thread .stop, but doesn't provide a mechanism for setting the message. I think it's more important to specify the exact cause of the exception, that a SIGINT was caught, than to use a possibly more accurate exception type, so the new patch throws a plain Error.
Hide
Permalink
george jahad added a comment -

Looks good to me.

Show
george jahad added a comment - Looks good to me.
Hide
Permalink
Chouser added a comment -

I just realized that since this will be in clojure.repl now, clojure.main could do (set-break-handler!) in repl-opt so that plain terminal REPLs would catch Ctrl-C by default. Would this be desirable? Hm, perhaps that warrants a separate issue.

Show
Chouser added a comment - I just realized that since this will be in clojure.repl now, clojure.main could do (set-break-handler!) in repl-opt so that plain terminal REPLs would catch Ctrl-C by default. Would this be desirable? Hm, perhaps that warrants a separate issue.
Hide
Permalink
Stuart Halloway added a comment -

Second patch is good. Let's consider repl-opt feature separately.

Show
Stuart Halloway added a comment - Second patch is good. Let's consider repl-opt feature separately.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved:
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.