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

Hide
Assembla Importer added a comment -
Show
Assembla Importer added a comment - Converted from http://www.assembla.com/spaces/clojure/tickets/460
Hide
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
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
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
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
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
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
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
george jahad added a comment -

Looks good to me.

Show
george jahad added a comment - Looks good to me.
Hide
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
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: