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

Receive SIGINT only in RunInteractiveShell #3357

Merged
merged 3 commits into from
Jun 28, 2024

Conversation

niten94
Copy link
Contributor

@niten94 niten94 commented Jun 17, 2024

There can be more than 1 channel receiving signals added so SIGINT signal handlers are reset before temporarily adding a channel in shell.RunInteractiveShell in this pull request. SIGINT is sent to the foreground process group of the terminal where CTRL+C is pressed, so the process is not killed in micro in the pull request.

There was another bug where micro crashes if the process was not able to be started and CTRL+C is pressed when "Press enter to continue" is displayed. The process is null if there was an error when starting it so micro was crashing when it is killed in the goroutine.

The error with starting the process is also returned instead if there is one that occured.

Fixes #2612

Temporarily reset SIGINT signal handlers and receive SIGINT in
RunInteractiveShell. Do not try to kill the process in micro when signal
is received.
Copy link
Contributor

@Andriamanitra Andriamanitra left a comment

Choose a reason for hiding this comment

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

I don't really understand how this works but I tested it and can confirm #2612 is fixed. I recall having a lot of trouble trying to fix the issue myself without messing up SIGINT handling in micro itself. Nice work!

@dmaluka
Copy link
Collaborator

dmaluka commented Jun 18, 2024

Thanks a lot, this has been a nasty issue.

The fix looks correct to me and works well in my tests (for example with my nav plugin, when I'm grepping in a huge directory, which takes long, and interrupting the grep via Ctrl-c).

Along the way I've noticed that the original issue #2085 that was "fixed" by this original signal handler is not really fully fixed, so I've reopened it. That original fix 0851499 looks suspicious, I don't quite understand its implications, I feel we should replace it with something better.

cmd.Start()
err = cmd.Wait()
err = cmd.Start()
if err == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Print error if it failed?

I've just tried this, seems to work ok (e.g. when trying to run RunInteractiveShell() with a non-existing command):

--- a/internal/shell/shell.go
+++ b/internal/shell/shell.go
@@ -104,6 +104,8 @@ func RunInteractiveShell(input string, wait bool, getOutput bool) (string, error
 	err = cmd.Start()
 	if err == nil {
 		err = cmd.Wait()
+	} else {
+		screen.TermMessage(err)
 	}
 
 	output := outputBytes.String()

Copy link
Contributor Author

@niten94 niten94 Jun 21, 2024

Choose a reason for hiding this comment

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

I think it can be a bit confusing having to press enter key 2 times if wait is true, but I will change it so that the error is printed and enter key has to be pressed even if wait is false when an error occurs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point. But with your version it prints an extra empty string if wait is false. What about:

		if (!wait) {
			screen.TermMessage(err)
		} else {
			fmt.Println(err)
		}

...Or actually it seems cleaner to move the "main" screen.TermMessage() code to the successful branch of the if. Then it would be just:

	err = cmd.Start()
	if err == nil {
		err = cmd.Wait()
		if wait {
			// This is just so we don't return right away and let the user press enter to return
			screen.TermMessage("")
		}
	} else {
		screen.TermMessage(err)
	}

Copy link
Contributor Author

@niten94 niten94 Jun 22, 2024

Choose a reason for hiding this comment

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

I have only seen error messages with 1 line in micro so I think it does look weird if another empty line is added. I will move the part with screen.TermMessage("") to the successful branch of the if statement.

@niten94
Copy link
Contributor Author

niten94 commented Jun 21, 2024

That original fix 0851499 looks suspicious, I don't quite understand its implications, I feel we should replace it with something better.

I have not seen how signals are handled in programs where files are written so I do not know how it can be changed. There can be a hang before signals are checked but I wrote about it in a comment in the issue you mentioned: #2085 (comment)

Print and return error with process start in RunInteractiveShell if
process was not able to be started. Wait until enter is pressed even if
`wait` is false.

Co-authored-by: Dmitry Maluka <[email protected]>
@dmaluka dmaluka merged commit dc77592 into zyedidia:master Jun 28, 2024
3 checks passed
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.

Keyboard interrupt (SIGINT) during a shell.RunInteractiveShell call from a plugin stops the editor itself
4 participants