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

Reforeground debugger process after debugge exits #3919

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ElectricPulse
Copy link

@ElectricPulse ElectricPulse commented Feb 17, 2025

I am debugging a bubbletea application and a lot of times I would love to mash CTRL-C to exit the application and then the server (that's two SIGINT signals). This very crude commit allows that. I also added a TODO for the windows side, because there the debugger needs to notify (via a channel) the debug command to start capturing signals.
One other thing that should probably be implemented is that client should exit on a headless server's exit.

Copy link
Member

@derekparker derekparker left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! Just needs some more work to be accepted.


// Process exited so we should "re-foreground" the main process, so that it can capture interrupts
if errors.As(err, &errProcessExited) && !tcxpgrp.IsForeground() {
pgid := syscall.Getpgrp()
Copy link
Member

Choose a reason for hiding this comment

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

The body of this branch can be folded into the branch below, I think.

Also, this functionality must be encapsulated somewhere else in a unix specific file, with another implementation available for Windows (can be stubbed initially if you don't plan to fix this for Windows in this PR). Can you place place this code in a function maybe within pkg/terminal/ somewhere with proper build tags via file name for unix/windows, might even be worth creating another small subpackage of terminal, but perhaps not. Then you can import and call here.

The reason is this layer of the code is supposed to be platform agnostic as much as possible.

@aarzilli
Copy link
Member

A few things:

  • The build is currently failing on macOS and windows.
  • service/debugger is probably the wrong place to do this, it should probably be pkg/proc/native.exitGuard
  • doing it unconditionally if IsForeground fails is wrong, maybe we never were the foreground process, it should only be done if pkg/proc/native.Launch was passed proc.LaunchForeground as a flag
  • I'd rather not drag in an external dependency just for a single line function like tcsetpgrp

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