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

SIGCHLD handler registration for grand-children processes #154

Closed
Mickael-van-der-Beek opened this issue Jan 22, 2015 · 7 comments
Closed

Comments

@Mickael-van-der-Beek
Copy link

This is a cross-post of an issue opened on the IO.js project: nodejs/build-containers#19.
It is related to grand-children reaping in cases where no init script that would automatically do the reaping is available.

The issue comes from the fact that libuv unregisters the SIGCHLD signal handler if a grand-children process's parent is destroyed.

C is not my strong suit but I'll look into the code of:

https://github.com/libuv/libuv/blob/v1.x/src/unix/process.c

this evening and try to come up with a fix.

[1] Original article:

http://blog.phusion.nl/2015/01/20/docker-and-the-pid-1-zombie-reaping-problem/

@bnoordhuis
Copy link
Member

Note that it's not possible to change any of the structs in include/ because that would make it an ABI change.

I suggest adding a new static struct sigaction sigchld_act in src/unix/process.c and use that to store the old SIGCHLD handler. Just make sure that you only reference it when holding the signal lock.

Technically, we'd be in UB land when the old handler is installed with signal() instead of sigaction() but I don't think it matters much in practice. POSIX allows for completely separate signal() and sigaction() implementations but AFAIK the platforms we support all use a single mechanism under the hood.

@saghul
Copy link
Member

saghul commented Jan 22, 2015

@bnoordhuis we do have those 4 reserved fields, in case the world is about to end ;-)

@bnoordhuis
Copy link
Member

@saghul Right, that's true. We could malloc the struct sigaction for now.

@Mickael-van-der-Beek If you go down that path, uv__handle_init() in src/uv-common.h should NULL the reserved[] fields.

@tarruda
Copy link

tarruda commented Jan 23, 2015

This SO answer suggests that double forking is a good way to prevent child processes from ever becoming zombies, though it seems to mess the process tree.

@tarruda
Copy link

tarruda commented Jan 23, 2015

This program reproduces the problem:

#include <stdlib.h>
#include <stdio.h>

#include <unistd.h>
#include <signal.h>
#include <sys/wait.h>

#include <uv.h>


char *args[] = {"cat", "-", NULL};
char to_write[] = "hello!";
uv_process_t proc = {0};
uv_process_options_t opts;
uv_stdio_container_t stdio[3];
uv_pipe_t in, out;
uv_write_t req;
uv_buf_t buffer;

static void alloc_cb(uv_handle_t *handle, size_t suggested, uv_buf_t *buf) {
  buf->len = suggested;
  buf->base = malloc(suggested);
}

static void read_cb(uv_stream_t *stream, ssize_t cnt, const uv_buf_t *buf) {
  fprintf(stderr, "\"%s\" sent: %s\n", args[0], buf->base);
  free(buf->base);
  uv_read_stop(stream);
  // kill(proc.pid, SIGTERM);
  uv_close((uv_handle_t *)&in, NULL);
  uv_close((uv_handle_t *)&out, NULL);
  uv_close((uv_handle_t *)&proc, NULL);
}

static void exit_cb(uv_process_t *proc, int64_t status, int term_signal)
{
  fprintf(stderr, "\"%s\" exited\n", args[0]);
}

static void write_cb(uv_write_t *req, int status) {
  fprintf(stderr, "\"%s\" stdin closed\n", args[0]);
}

int main(int argc, char *argv[]) {
  int status;
  opts.file = args[0];
  opts.args = args;
  opts.stdio = stdio;
  opts.stdio_count = 3;
  opts.flags = 0;
  opts.exit_cb = exit_cb;
  opts.cwd = NULL;
  opts.env = NULL;

  uv_pipe_init(uv_default_loop(), &in, 0);
  stdio[0].flags = UV_CREATE_PIPE | UV_READABLE_PIPE;
  stdio[0].data.stream = (uv_stream_t *)&in;

  uv_pipe_init(uv_default_loop(), &out, 0);
  stdio[1].flags = UV_CREATE_PIPE | UV_WRITABLE_PIPE;
  stdio[1].data.stream = (uv_stream_t *)&out;

  stdio[2].flags = UV_IGNORE;
  stdio[2].data.fd = 2;

  if ((status = uv_spawn(uv_default_loop(), &proc, &opts))) {
    fprintf(stderr, "spawn error: %s\n", uv_strerror(status));
    return status;
  }

  uv_read_start((uv_stream_t *)&out, alloc_cb, read_cb);
  buffer.base = to_write;
  buffer.len = sizeof(to_write);
  uv_write(&req, (uv_stream_t *)&in, &buffer, 1, write_cb);
  uv_run(uv_default_loop(), UV_RUN_DEFAULT);
  // while (!kill(proc.pid, 0)) {
  //   waitpid(proc.pid, &status, WNOHANG);
  // }
  sleep(10);
  return 0;
}

Running this program then ps -e | grep cat will show the zombie for 10 seconds. If the last loop is uncommented, the zombie is killed properly.

Some observations:

  • The exit_cb is never called, even though the child process has exited
  • Sending a terminating signal won't result in a zombie and exit_cb is called normally(uncomment the kill call in read_cb)

@stale
Copy link

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 10, 2019
@vtjnash
Copy link
Member

vtjnash commented Aug 12, 2019

I'm not sure about OP issue, but tarruda's code sample is documentation issue #1911

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

No branches or pull requests

5 participants