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

Fix sigint handling when running in helper mode #869

Merged
merged 7 commits into from
Jun 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions cli_helper/kart.c
Original file line number Diff line number Diff line change
Expand Up @@ -209,18 +209,14 @@ void exit_on_sigalrm(int sig)
exit(exit_code);
}

char *socket_filename;

/**
* @brief Exit signal handler for SIGINT
* @brief Exit signal handler for SIGINT.
* Tries to kill the whole process group.
*/
void exit_on_sigint(int sig)
{
putchar('\n');
rcoup marked this conversation as resolved.
Show resolved Hide resolved
const char* kill_cmd_template = "lsof -t '%s' | xargs kill";
char* kill_cmd = malloc(strlen(kill_cmd_template) + strlen(socket_filename) + 2);
sprintf(kill_cmd, kill_cmd_template, socket_filename);
system(kill_cmd);
killpg(0, sig);
exit(128 + sig);
}

Expand All @@ -236,6 +232,10 @@ int main(int argc, char **argv, char **environ)
{
debug("enabled %s, pid=%d\n", cmd_path, getpid());

// Make this process the leader of a process group:
// The procress-group ID (pgid) will be the same as the pid.
setpgrp();
rcoup marked this conversation as resolved.
Show resolved Hide resolved

// start or use an existing helper process
char **env_ptr;

Expand Down Expand Up @@ -282,7 +282,7 @@ int main(int argc, char **argv, char **environ)
int fp = open(getcwd(NULL, 0), O_RDONLY);
int fds[4] = {fileno(stdin), fileno(stdout), fileno(stderr), fp};

socket_filename = malloc(strlen(getenv("HOME")) + strlen(".kart.socket") + 2);
char *socket_filename = malloc(strlen(getenv("HOME")) + strlen(".kart.socket") + 2);
sprintf(socket_filename, "%s/%s", getenv("HOME"), ".kart.socket");
int socket_fd = socket(AF_UNIX, SOCK_STREAM, 0);

Expand Down
2 changes: 1 addition & 1 deletion kart/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ def _cleanup_process_group(signum, stack_frame):
return
_kart_process_group_killed = True
try:
os.killpg(os.getpid(), signum)
os.killpg(0, signum)
except Exception:
rcoup marked this conversation as resolved.
Show resolved Hide resolved
pass
sys.exit(128 + signum)
Expand Down
11 changes: 11 additions & 0 deletions kart/helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ def helper(ctx, socket_filename, timeout, args):
else:
# child
_helper_log("post-fork")

payload, fds = recv_json_and_fds(client, maxfds=4)
if not payload or len(fds) != 4:
click.echo(
Expand Down Expand Up @@ -169,6 +170,16 @@ def helper(ctx, socket_filename, timeout, args):
f"Payload:\n{repr(payload)}",
)
else:
try:
# Join the process group of the calling process - so that if they get killed, we get killed to.
os.setpgid(0, calling_environment["pid"])
os.environ["_KART_PGID_SET"] = "1"
except Exception as e:
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this could catch a more specific exception class (OSError? or one of the subclasses: https://docs.python.org/3/library/exceptions.html#os-exceptions)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the possible error codes from setpgid() are all mapped to subclasses of OSError.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
except Exception as e:
except OSError as e:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

# Kart will still work even if this fails: it just means SIGINT Ctrl+C might not work properly.
# We'll just log it and hope for the best.
_helper_log(f"error joining caller's process group: {e}")
pass

sys.argv[1:] = calling_environment["argv"][1:]
_helper_log(f"cmd={' '.join(calling_environment['argv'])}")
os.environ.clear()
Expand Down
Loading