From d4547511735190a81449727a89dc90f6ef1d99a3 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sat, 24 Nov 2012 00:24:11 -0800 Subject: [PATCH] Revert recent change for Bug#8855. As reported by Harald Hanche-Olsen in the change introduces a further bug, of creating lots of zombie processes in some cases. Further work is needed to come up with a better fix for Bug#8855. --- nt/ChangeLog | 4 + nt/config.nt | 2 +- nt/inc/ms-w32.h | 3 + nt/inc/sys/wait.h | 33 -------- src/ChangeLog | 9 +++ src/process.c | 191 +++++++++++++++++++++++++--------------------- src/process.h | 3 - src/sysdep.c | 7 +- src/w32proc.c | 127 +++++++++--------------------- 9 files changed, 162 insertions(+), 217 deletions(-) delete mode 100644 nt/inc/sys/wait.h diff --git a/nt/ChangeLog b/nt/ChangeLog index 63c8ba8b701..20789c75ec3 100644 --- a/nt/ChangeLog +++ b/nt/ChangeLog @@ -1,3 +1,7 @@ +2012-11-24 Paul Eggert + + Revert recent change for Bug#8855; see ../src/ChangeLog. + 2012-11-23 Eli Zaretskii Fix a race condition with glib (Bug#8855). diff --git a/nt/config.nt b/nt/config.nt index 7e82283b41a..ed1cddf1e12 100644 --- a/nt/config.nt +++ b/nt/config.nt @@ -963,7 +963,7 @@ along with GNU Emacs. If not, see . */ #undef HAVE_SYS_VLIMIT_H /* Define to 1 if you have that is POSIX.1 compatible. */ -#define HAVE_SYS_WAIT_H 1 +#undef HAVE_SYS_WAIT_H /* Define to 1 if you have the header file. */ #undef HAVE_TERM_H diff --git a/nt/inc/ms-w32.h b/nt/inc/ms-w32.h index a9a3049f9b0..dd2ae781cb8 100644 --- a/nt/inc/ms-w32.h +++ b/nt/inc/ms-w32.h @@ -185,12 +185,15 @@ extern char *getenv (); /* Subprocess calls that are emulated. */ #define spawnve sys_spawnve +#define wait sys_wait #define kill sys_kill #define signal sys_signal /* Internal signals. */ #define emacs_raise(sig) emacs_abort() +extern int sys_wait (int *); + /* termcap.c calls that are emulated. */ #define tputs sys_tputs #define tgetstr sys_tgetstr diff --git a/nt/inc/sys/wait.h b/nt/inc/sys/wait.h deleted file mode 100644 index 8d890c9e175..00000000000 --- a/nt/inc/sys/wait.h +++ /dev/null @@ -1,33 +0,0 @@ -/* A limited emulation of sys/wait.h on Posix systems. - -Copyright (C) 2012 Free Software Foundation, Inc. - -This file is part of GNU Emacs. - -GNU Emacs is free software: you can redistribute it and/or modify -it under the terms of the GNU General Public License as published by -the Free Software Foundation, either version 3 of the License, or -(at your option) any later version. - -GNU Emacs is distributed in the hope that it will be useful, -but WITHOUT ANY WARRANTY; without even the implied warranty of -MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -GNU General Public License for more details. - -You should have received a copy of the GNU General Public License -along with GNU Emacs. If not, see . */ - -#ifndef INC_SYS_WAIT_H_ -#define INC_SYS_WAIT_H_ - -#define WNOHANG 1 -#define WUNTRACED 2 -#define WSTOPPED 2 /* same as WUNTRACED */ -#define WEXITED 4 -#define WCONTINUED 8 - -/* The various WIF* macros are defined in src/syswait.h. */ - -extern pid_t waitpid (pid_t, int *, int); - -#endif /* INC_SYS_WAIT_H_ */ diff --git a/src/ChangeLog b/src/ChangeLog index 6c9893b2f4f..99abda8a884 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,12 @@ +2012-11-24 Paul Eggert + + Revert recent change for Bug#8855. + As reported by Harald Hanche-Olsen in + + the change introduces a further bug, of creating lots of zombie + processes in some cases. Further work is needed to come up with a + better fix for Bug#8855. + 2012-11-24 Eli Zaretskii * xdisp.c (draw_glyphs): Don't draw in mouse face if mouse diff --git a/src/process.c b/src/process.c index c095d13293b..77e99ead01f 100644 --- a/src/process.c +++ b/src/process.c @@ -130,6 +130,14 @@ extern int sys_select (int, SELECT_TYPE *, SELECT_TYPE *, SELECT_TYPE *, EMACS_TIME *, void *); #endif +#ifndef WNOHANG +# undef waitpid +# define waitpid(pid, status, options) wait (status) +#endif +#ifndef WUNTRACED +# define WUNTRACED 0 +#endif + /* Work around GCC 4.7.0 bug with strict overflow checking; see . These lines can be removed once the GCC bug is fixed. */ @@ -787,8 +795,9 @@ get_process (register Lisp_Object name) #ifdef SIGCHLD /* Fdelete_process promises to immediately forget about the process, but in reality, Emacs needs to remember those processes until they have been - treated by the SIGCHLD handler and waitpid has been invoked on them; - otherwise they might fill up the kernel's process table. */ + treated by the SIGCHLD handler; otherwise this handler would consider the + process as being synchronous and say that the synchronous process is + dead. */ static Lisp_Object deleted_pid_list; #endif @@ -1695,7 +1704,16 @@ create_process (Lisp_Object process, char **new_argv, Lisp_Object current_dir) if (inchannel > max_process_desc) max_process_desc = inchannel; - /* This may signal an error. */ + /* Until we store the proper pid, enable the SIGCHLD handler + to recognize an unknown pid as standing for this process. + It is very important not to let this `marker' value stay + in the table after this function has returned; if it does + it might cause call-process to hang and subsequent asynchronous + processes to get their return values scrambled. */ + XPROCESS (process)->pid = -1; + + /* This must be called after the above line because it may signal an + error. */ setup_process_coding_systems (process); encoded_current_dir = ENCODE_FILE (current_dir); @@ -1862,8 +1880,6 @@ create_process (Lisp_Object process, char **new_argv, Lisp_Object current_dir) #endif XPROCESS (process)->pid = pid; - if (0 <= pid) - XPROCESS (process)->alive = 1; /* Stop blocking signals in the parent. */ #ifdef SIGCHLD @@ -6257,35 +6273,9 @@ process has been transmitted to the serial port. */) return process; } -/* If the status of the process DESIRED has changed, return true and - set *STATUS to its exit status; otherwise, return false. - If HAVE is nonnegative, assume that HAVE = waitpid (HAVE, STATUS, ...) - has already been invoked, and do not invoke waitpid again. */ - -static bool -process_status_retrieved (pid_t desired, pid_t have, int *status) -{ - if (have < 0) - { - /* Invoke waitpid only with a known process ID; do not invoke - waitpid with a nonpositive argument. Otherwise, Emacs might - reap an unwanted process by mistake. For example, invoking - waitpid (-1, ...) can mess up glib by reaping glib's subprocesses, - so that another thread running glib won't find them. */ - do - have = waitpid (desired, status, WNOHANG | WUNTRACED); - while (have < 0 && errno == EINTR); - } - - return have == desired; -} - -/* If PID is nonnegative, the child process PID with wait status W has - changed its status; record this and return true. - - If PID is negative, ignore W, and look for known child processes - of Emacs whose status have changed. For each one found, record its new - status. +/* On receipt of a signal that a child status has changed, loop asking + about children with changed statuses until the system says there + are no more. All we do is change the status; we do not run sentinels or print notifications. That is saved for the next time keyboard input is @@ -6308,15 +6298,13 @@ process_status_retrieved (pid_t desired, pid_t have, int *status) ** Malloc WARNING: This should never call malloc either directly or indirectly; if it does, that is a bug */ +/* Record the changed status of the child process PID with wait status W. */ void record_child_status_change (pid_t pid, int w) { #ifdef SIGCHLD - - /* On POSIXish hosts, record at most one child only if we already - know one child that has exited. */ - bool record_at_most_one_child = 0 <= pid; - + Lisp_Object proc; + struct Lisp_Process *p; Lisp_Object tail; /* Find the process that signaled us, and record its status. */ @@ -6324,69 +6312,68 @@ record_child_status_change (pid_t pid, int w) /* The process can have been deleted by Fdelete_process. */ for (tail = deleted_pid_list; CONSP (tail); tail = XCDR (tail)) { - bool all_pids_are_fixnums - = (MOST_NEGATIVE_FIXNUM <= TYPE_MINIMUM (pid_t) - && TYPE_MAXIMUM (pid_t) <= MOST_POSITIVE_FIXNUM); Lisp_Object xpid = XCAR (tail); - if (all_pids_are_fixnums ? INTEGERP (xpid) : NUMBERP (xpid)) + if ((INTEGERP (xpid) && pid == XINT (xpid)) + || (FLOATP (xpid) && pid == XFLOAT_DATA (xpid))) { - pid_t deleted_pid; - if (INTEGERP (xpid)) - deleted_pid = XINT (xpid); - else - deleted_pid = XFLOAT_DATA (xpid); - if (process_status_retrieved (deleted_pid, pid, &w)) - { - XSETCAR (tail, Qnil); - if (record_at_most_one_child) - return; - } + XSETCAR (tail, Qnil); + return; } } /* Otherwise, if it is asynchronous, it is in Vprocess_alist. */ + p = 0; for (tail = Vprocess_alist; CONSP (tail); tail = XCDR (tail)) { - Lisp_Object proc = XCDR (XCAR (tail)); - struct Lisp_Process *p = XPROCESS (proc); - if (p->alive && process_status_retrieved (p->pid, pid, &w)) - { - /* Change the status of the process that was found. */ - p->tick = ++process_tick; - p->raw_status = w; - p->raw_status_new = 1; + proc = XCDR (XCAR (tail)); + p = XPROCESS (proc); + if (EQ (p->type, Qreal) && p->pid == pid) + break; + p = 0; + } - /* If process has terminated, stop waiting for its output. */ - if (WIFSIGNALED (w) || WIFEXITED (w)) - { - int clear_desc_flag = 0; - p->alive = 0; - if (p->infd >= 0) - clear_desc_flag = 1; + /* Look for an asynchronous process whose pid hasn't been filled + in yet. */ + if (! p) + for (tail = Vprocess_alist; CONSP (tail); tail = XCDR (tail)) + { + proc = XCDR (XCAR (tail)); + p = XPROCESS (proc); + if (p->pid == -1) + break; + p = 0; + } - /* clear_desc_flag avoids a compiler bug in Microsoft C. */ - if (clear_desc_flag) - { - FD_CLR (p->infd, &input_wait_mask); - FD_CLR (p->infd, &non_keyboard_wait_mask); - } - } + /* Change the status of the process that was found. */ + if (p) + { + int clear_desc_flag = 0; - /* Tell wait_reading_process_output that it needs to wake up and - look around. */ - if (input_available_clear_time) - *input_available_clear_time = make_emacs_time (0, 0); + p->tick = ++process_tick; + p->raw_status = w; + p->raw_status_new = 1; + + /* If process has terminated, stop waiting for its output. */ + if ((WIFSIGNALED (w) || WIFEXITED (w)) + && p->infd >= 0) + clear_desc_flag = 1; - if (record_at_most_one_child) - return; + /* We use clear_desc_flag to avoid a compiler bug in Microsoft C. */ + if (clear_desc_flag) + { + FD_CLR (p->infd, &input_wait_mask); + FD_CLR (p->infd, &non_keyboard_wait_mask); } - } - if (0 <= pid) + /* Tell wait_reading_process_output that it needs to wake up and + look around. */ + if (input_available_clear_time) + *input_available_clear_time = make_emacs_time (0, 0); + } + /* There was no asynchronous process found for that pid: we have + a synchronous process. */ + else { - /* The caller successfully waited for a pid but no asynchronous - process was found for it, so this is a synchronous process. */ - synch_process_alive = 0; /* Report the status of the synchronous process. */ @@ -6405,10 +6392,38 @@ record_child_status_change (pid_t pid, int w) #ifdef SIGCHLD +/* On some systems, the SIGCHLD handler must return right away. If + any more processes want to signal us, we will get another signal. + Otherwise, loop around to use up all the processes that have + something to tell us. */ +#if (defined WINDOWSNT \ + || (defined USG && !defined GNU_LINUX \ + && !(defined HPUX && defined WNOHANG))) +enum { CAN_HANDLE_MULTIPLE_CHILDREN = 0 }; +#else +enum { CAN_HANDLE_MULTIPLE_CHILDREN = 1 }; +#endif + static void handle_child_signal (int sig) { - record_child_status_change (-1, 0); + do + { + pid_t pid; + int status; + + do + pid = waitpid (-1, &status, WNOHANG | WUNTRACED); + while (pid < 0 && errno == EINTR); + + /* PID == 0 means no processes found, PID == -1 means a real failure. + Either way, we have done all our job. */ + if (pid <= 0) + break; + + record_child_status_change (pid, status); + } + while (CAN_HANDLE_MULTIPLE_CHILDREN); } static void diff --git a/src/process.h b/src/process.h index 74d1a124060..ce3d2e702cc 100644 --- a/src/process.h +++ b/src/process.h @@ -142,9 +142,6 @@ struct Lisp_Process /* Flag to set coding-system of the process buffer from the coding_system used to decode process output. */ unsigned int inherit_coding_system_flag : 1; - /* Whether the process is alive, i.e., can be waited for. Running - processes can be waited for, but exited and fake processes cannot. */ - unsigned int alive : 1; /* Record the process status in the raw form in which it comes from `wait'. This is to avoid consing in a signal handler. The `raw_status_new' flag indicates that `raw_status' contains a new status that still diff --git a/src/sysdep.c b/src/sysdep.c index bb81353847b..63eac5d9e09 100644 --- a/src/sysdep.c +++ b/src/sysdep.c @@ -289,6 +289,10 @@ wait_for_termination_1 (pid_t pid, int interruptible) { while (1) { +#ifdef WINDOWSNT + wait (0); + break; +#else /* not WINDOWSNT */ int status; int wait_result = waitpid (pid, &status, 0); if (wait_result < 0) @@ -302,8 +306,7 @@ wait_for_termination_1 (pid_t pid, int interruptible) break; } - /* Note: the MS-Windows emulation of waitpid calls QUIT - internally. */ +#endif /* not WINDOWSNT */ if (interruptible) QUIT; } diff --git a/src/w32proc.c b/src/w32proc.c index b4f2099f06a..e3c54fe5460 100644 --- a/src/w32proc.c +++ b/src/w32proc.c @@ -783,6 +783,7 @@ alarm (int seconds) /* Child process management list. */ int child_proc_count = 0; child_process child_procs[ MAX_CHILDREN ]; +child_process *dead_child = NULL; static DWORD WINAPI reader_thread (void *arg); @@ -1035,6 +1036,9 @@ create_child (char *exe, char *cmdline, char *env, int is_gui_app, if (cp->pid < 0) cp->pid = -cp->pid; + /* pid must fit in a Lisp_Int */ + cp->pid = cp->pid & INTMASK; + *pPid = cp->pid; return TRUE; @@ -1110,110 +1114,55 @@ reap_subprocess (child_process *cp) delete_child (cp); } -/* Wait for a child process specified by PID, or for any of our - existing child processes (if PID is nonpositive) to die. When it - does, close its handle. Return the pid of the process that died - and fill in STATUS if non-NULL. */ +/* Wait for any of our existing child processes to die + When it does, close its handle + Return the pid and fill in the status if non-NULL. */ -pid_t -waitpid (pid_t pid, int *status, int options) +int +sys_wait (int *status) { DWORD active, retval; int nh; + int pid; child_process *cp, *cps[MAX_CHILDREN]; HANDLE wait_hnd[MAX_CHILDREN]; - DWORD timeout_ms; - int dont_wait = (options & WNOHANG) != 0; nh = 0; - /* According to Posix: - - PID = -1 means status is requested for any child process. - - PID > 0 means status is requested for a single child process - whose pid is PID. - - PID = 0 means status is requested for any child process whose - process group ID is equal to that of the calling process. But - since Windows has only a limited support for process groups (only - for console processes and only for the purposes of passing - Ctrl-BREAK signal to them), and since we have no documented way - of determining whether a given process belongs to our group, we - treat 0 as -1. - - PID < -1 means status is requested for any child process whose - process group ID is equal to the absolute value of PID. Again, - since we don't support process groups, we treat that as -1. */ - if (pid > 0) + if (dead_child != NULL) { - int our_child = 0; - - /* We are requested to wait for a specific child. */ - for (cp = child_procs + (child_proc_count-1); cp >= child_procs; cp--) - { - /* Some child_procs might be sockets; ignore them. Also - ignore subprocesses whose output is not yet completely - read. */ - if (CHILD_ACTIVE (cp) - && cp->procinfo.hProcess - && cp->pid == pid) - { - our_child = 1; - break; - } - } - if (our_child) - { - if (cp->fd < 0 || (fd_info[cp->fd].flags & FILE_AT_EOF) != 0) - { - wait_hnd[nh] = cp->procinfo.hProcess; - cps[nh] = cp; - nh++; - } - else if (dont_wait) - { - /* PID specifies our subprocess, but its status is not - yet available. */ - return 0; - } - } - if (nh == 0) - { - /* No such child process, or nothing to wait for, so fail. */ - errno = ECHILD; - return -1; - } + /* We want to wait for a specific child */ + wait_hnd[nh] = dead_child->procinfo.hProcess; + cps[nh] = dead_child; + if (!wait_hnd[nh]) emacs_abort (); + nh++; + active = 0; + goto get_result; } else { for (cp = child_procs + (child_proc_count-1); cp >= child_procs; cp--) - { - if (CHILD_ACTIVE (cp) - && cp->procinfo.hProcess - && (cp->fd < 0 || (fd_info[cp->fd].flags & FILE_AT_EOF) != 0)) - { - wait_hnd[nh] = cp->procinfo.hProcess; - cps[nh] = cp; - nh++; - } - } - if (nh == 0) - { - /* Nothing to wait on, so fail. */ - errno = ECHILD; - return -1; - } + /* some child_procs might be sockets; ignore them */ + if (CHILD_ACTIVE (cp) && cp->procinfo.hProcess + && (cp->fd < 0 || (fd_info[cp->fd].flags & FILE_AT_EOF) != 0)) + { + wait_hnd[nh] = cp->procinfo.hProcess; + cps[nh] = cp; + nh++; + } } - if (dont_wait) - timeout_ms = 0; - else - timeout_ms = 1000; /* check for quit about once a second. */ + if (nh == 0) + { + /* Nothing to wait on, so fail */ + errno = ECHILD; + return -1; + } do { + /* Check for quit about once a second. */ QUIT; - active = WaitForMultipleObjects (nh, wait_hnd, FALSE, timeout_ms); + active = WaitForMultipleObjects (nh, wait_hnd, FALSE, 1000); } while (active == WAIT_TIMEOUT); if (active == WAIT_FAILED) @@ -1243,10 +1192,8 @@ get_result: } if (retval == STILL_ACTIVE) { - /* Should never happen. */ + /* Should never happen */ DebPrint (("Wait.WaitForMultipleObjects returned an active process\n")); - if (pid > 0 && dont_wait) - return 0; errno = EINVAL; return -1; } @@ -1260,8 +1207,6 @@ get_result: else retval <<= 8; - if (pid > 0 && active != 0) - emacs_abort (); cp = cps[active]; pid = cp->pid; #ifdef FULL_DEBUG @@ -2050,7 +1995,9 @@ count_children: DebPrint (("select calling SIGCHLD handler for pid %d\n", cp->pid)); #endif + dead_child = cp; sig_handlers[SIGCHLD] (SIGCHLD); + dead_child = NULL; } } else if (fdindex[active] == -1) -- 2.39.2