Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add POSIX compliant ctrl-c handling #912

Merged
merged 5 commits into from
Dec 14, 2019

Conversation

sloane-shark
Copy link
Contributor

@sloane-shark sloane-shark commented Oct 29, 2019

first pass at addressing #872

based on/cribbed from ghci

the main contribution of this feature is more user-friendly signal handling, particularly making it so that ucm responds to SIGINT similarly to other shells, like ghci. if you're typing some input and hit ^c, you will now cancel that input and be given a new line to type on. you can also cancel execution/evaluation of running code in ucm the same way. ^d will emit an EOF that will kill the shell the way ^c used to.

this only handles posix signals at the moment, but could be feasibly be extended with the same (CPP-based) windows handling that ghci uses.

i haven't made many contributions to codebases this size, so let me know if some of this would be better added somewhere else or packaged up in a more modular way.

@aryairani
Copy link
Contributor

aryairani commented Nov 26, 2019

Thanks @matthewess, LGTM. Didn't notice it was ready for review.

For all of us who haven't looked closely at the issue and maybe haven't tried it out, could you add a comment in the PR to describe:

  • How specifically does it change the behavior of ucm & how can we tell it's doing what it's expected to: e.g., is there a reproducible case that it handles better?
  • The PR description calls it a "first-pass" — what would you say is left to do? Just the Windows port, eventually, or something more?

Thanks again!

@mitchellwrosen
Copy link
Member

mitchellwrosen commented Dec 6, 2019

LGTM, I think this is ready to merge once conflicts are resolved.

@aryairani, in a nutshell, it changes the default behavior of the GHC runtime (translate the first SIGINT into a UserInterrupt delivered to the main thread) into ghci-like behavior (translate every SIGINT into a UserInterrupt delivered to the main thread).

Elsewhere in the commandline main you're catching UserInterrupt and re-polling for user input (hence why you currently get the nice re-prompt behavior, but only once nvm, that was added in this patch, too).

x -> pure x
x -> pure x) `catch` interruptHandler
interruptHandler (asyncExceptionFromException -> Just UserInterrupt) = awaitInput
interruptHandler _ = pure $ Right QuitI
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would probably be more correct to re-throw non UserInterrupt exceptions rather than just quit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we wait on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could be fixed up post-merge, it's good enuf ;)

@sloane-shark
Copy link
Contributor Author

added a bit more context but @mitchellwrosen has got it. @aryairani yes, the remaining work is Windows compatibility, I believe. but as mitchell called out there should probably be more delicate handling of non UserInterrupt exceptions, as well.

@aryairani
Copy link
Contributor

Great! Thanks!

@aryairani aryairani merged commit 2adb598 into unisonweb:master Dec 14, 2019
@aryairani
Copy link
Contributor

If you wouldn’t mind creating tickets to track the remaining work, that would be awesome. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants