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

Dispatch interrupts to StdIn and ReadConsole #60

Merged
merged 3 commits into from
Jun 30, 2023

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Jun 29, 2023

Branched from #58
Addresses posit-dev/positron#535.

The fix is in two parts:

  • In ReadConsole(): Detect interrupts signaled during a user-prompt request, e.g. during readline(). In case of interrupt, quit the current Rust context and call onintr() to cause a longjump out of readline(). The longjump is called from a plain-old-frame function that doesn't have any destructors on the stack.

    The behaviour in that case is unspecified at the moment but it is also safe with the current implementation of rustc, and it should be made formally safe in a future version of rust: https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md. If that's a concern we could move our method to a .c file, but the implementation proposed in this PR seems safe enough.

  • Dispatch an interrupt message from Control to StdIn so that the latter doesn't wait for an input reply that is never coming. It was easiest to implement this with a simple channel, but this did add some complications inside StdIn because of the need to pump the channel continuously. This could be simplified in the future if we implement a pub/sub mechanism.

I noticed another race condition between interrupts and exec-request messages, now documented in comments. To fix it, we could manage the Shell and Control sockets on the common message event thread implemented in #58. The Control messages would need to be handled in a blocking way to ensure subscribers are notified before the next incoming message is processed.

@lionel- lionel- requested a review from jmcphers June 29, 2023 14:12
// interrupt notifications here and loop infinitely over them.
//
// This could be simplified by having a mechanism for
// subscribing and unsubscribing to a broadcasting channel. We
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we use bus for this elsewhere: https://crates.io/crates/bus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I've also found this implementation that suggests wrapping Crossbeam should be easy, if we need more features such as unboundedness: crossbeam-rs/crossbeam#374 (comment). I'll revisit this in the future if needed.

@lionel- lionel- force-pushed the refactor/stdin-channels branch from 12fc843 to 1a0cafc Compare June 30, 2023 10:18
@lionel- lionel- changed the base branch from refactor/stdin-channels to main June 30, 2023 10:24
@lionel- lionel- force-pushed the bugfix/readline-interrupt branch from 0a2117a to fc69e0c Compare June 30, 2023 10:34
@lionel- lionel- merged commit af701b0 into main Jun 30, 2023
@lionel- lionel- deleted the bugfix/readline-interrupt branch June 30, 2023 10:39
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.

2 participants