From kde-core-devel Wed Jul 07 18:28:35 1999 From: Harri Porten Date: Wed, 07 Jul 1999 18:28:35 +0000 To: kde-core-devel Subject: Please look at: [Fwd: Bug#1513: KProcess deadlock] X-MARC-Message: https://marc.info/?l=kde-core-devel&m=93137180812698 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--------------F69FD8AA4FD2EC6A8348405B" This is a multi-part message in MIME format. --------------F69FD8AA4FD2EC6A8348405B Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Hi ! Could anyone who has worked on KProcess so far take a look at bug#1513, please ? http://bugs.kde.org//db/15/1513.html Alex Hayward has discovered a bug that comes up when receiving larger amounts of data from the child process. He's even offering a patch but I'm too chicken to apply it* without others having looked at it. It's a very delicate part of libkdecore. Harri. * the patch may need some manual work since kprocess has slightly changed meanwhile --------------F69FD8AA4FD2EC6A8348405B Content-Type: message/rfc822 Content-Transfer-Encoding: 7bit Content-Disposition: inline Received: from rztsun.rz.tu-harburg.de (rztsun.rz.tu-harburg.de [134.28.200.14]) by hp00.rz.tu-harburg.de (8.8.6 (PHNE_14041)/8.8.6) with ESMTP id PAA20288 for ; Wed, 7 Jul 1999 15:07:34 +0200 (METDST) Received: from max.tat.physik.uni-tuebingen.de (max.tat.physik.uni-tuebingen.de [134.2.170.93]) by rztsun.rz.tu-harburg.de (8.9.0/8.8.8) with ESMTP id PAA14390 for ; Wed, 7 Jul 1999 15:07:33 +0200 (MET DST) Received: by max.tat.physik.uni-tuebingen.de id <741469-219>; Wed, 7 Jul 1999 15:06:29 +0200 Subject: Bug#1513: KProcess deadlock Reply-To: Alex Hayward , 1513@bugs.kde.org Resent-From: Alex Hayward Resent-To: kde-bugs-dist@alpha.tat.physik.uni-tuebingen.de Resent-CC: Kalle Dalheimer Resent-Date: Wed, 07 Jul 1999 13:03:01 GMT Resent-Message-ID: Resent-Fake-Sender: coolo@kde.org X-KDE-PR-Message: report 1513 X-KDE-PR-Package: kdelibs X-Loop: owner@bugs.kde.org Date: Tue, 6 Jul 1999 22:00:50 +0100 (BST) From: Alex Hayward To: Harri Porten cc: 1513@bugs.kde.org In-Reply-To: <377FA7D6.9B9EEA2A@tu-harburg.de> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Old-Return-Path: X-Orcpt: rfc822;kde-bugs-dist@alpha.tat.physik.uni-tuebingen.de Resent-Reply-To: kde-bugs-dist@max.tat.physik.uni-tuebingen.de X-Mailing-List: archive/latest/195 X-Loop: kde-bugs-dist@max.tat.physik.uni-tuebingen.de Precedence: list Resent-Sender: kde-bugs-dist-request@max.tat.physik.uni-tuebingen.de X-Mozilla-Status2: 00000000 On Sun, 4 Jul 1999, Harri Porten wrote: > > When the KProcess::Block flag is specified kprocess does the following: > > > > 1. Sets up sockets for communication with the child. > > 2. Forks and execs in the child. > > 3. Waits for the child to exit. > > 4. Reads to EOF from the sockets. > > > > This will deadlock if the child outputs enough data to fill the socket > > buffers. Under FreeBSD this occurs after 8k of data, under Linux it is > > somewhat higher (I think about 64k). > > I agree that there's a problem (your test program demonstrates this). > However, I'm not sure if I understand your patch correctly. Doesn't it > completely break communication with the child process by ignoring it ? > Please correct my if I'm wrong. It doesn't... but I can see that its wrong for other reason (the socket notifiers never get deleted in non-blocking mode). I've redone the patch a bit so that this doesn't happen. I've also fixed what I think is another potential deadlock---the child's stdout is read to EOF before stderr so that the child could block if the stderr socket buffer fills before stdout is closed. I had to use select() for that which I'm a little shaky on (I'm also not sure about its portability...) so that bit is probably worth checking carefully. None of it is heavily tested (it works OK with my test program and with kpackage, though). What the patch now (hopefully...) does is: - Make sure that socket notifiers are not set up and that the sockets are not marked non-blocking when in blocking mode. - After doing the fork/exec when in blocking mode call commClose to drain the socket buffers, then wait for the process to exit (coping with the case of the child's exit being caught by the SIGCHLD handler in kprocctrl.cpp), then perform the other actions of processHasExited (setting return status etc). - Adds the select change mentioned above. The patch is below. --- kprocess.cpp.orig Sun Jul 4 22:16:03 1999 +++ kprocess.cpp Tue Jul 6 21:50:43 1999 @@ -253,8 +253,14 @@ input_data = 0; if (run_mode == Block) { - waitpid(pid, &status, 0); - processHasExited(status); + commClose(); + + // Its possible that the child's exit was caught by the SIGCHLD handler + // which will have set status for us. + if (waitpid(pid, &status, 0) != -1) this->status = status; + + runs = FALSE; + emit processExited(this); } } free(arglist); @@ -425,7 +431,7 @@ len = ::read(fdno, buffer, 1024); - if ( 0 != len) + if ( 0 < len) emit receivedStderr(this, buffer, len); return len; } @@ -465,6 +471,10 @@ if (communication & Stderr) close(err[1]); + // Don't create socket notifiers and set the sockets non-blocking if + // blocking is requested. + if (run_mode == Block) return ok; + if (communication & Stdin) { ok &= (-1 != fcntl(in[1], F_SETFL, O_NONBLOCK)); innot = new QSocketNotifier(in[1], QSocketNotifier::Write, this); @@ -531,17 +541,58 @@ if (communication & Stdin) delete innot; + if ((communication & Stdout) && (communication & Stderr)) { + // If both channels are being read we need to make sure that one socket buffer + // doesn't fill up whilst we are waiting for data on the other (causing a deadlock). + // Hence we need to use select. + + // Once one or other of the channels has reached EOF (or given an error) go back + // to the usual mechanism. + + int fds_ready = 1; + fd_set rfds; + + fcntl(out[0], F_SETFL, O_NONBLOCK); + fcntl(err[0], F_SETFL, O_NONBLOCK); + + while (1) { + FD_ZERO(&rfds); + FD_SET(out[0], &rfds); + FD_SET(err[0], &rfds); + + fds_ready = select(QMAX(out[0], err[0]), &rfds, 0, 0, 0); + if (fds_ready <= 0) break; + + if (FD_ISSET(out[0], &rfds)) { + int ret = 0; + while (ret > 0) ret = childOutput(out[0]); + if ((ret == -1 && errno != EAGAIN) || ret == 0) break; + } + + if (FD_ISSET(err[0], &rfds)) { + int ret = 0; + while (ret > 0) ret = childError(err[0]); + if ((ret == -1 && errno != EAGAIN) || ret == 0) break; + } + } + + + fcntl(out[0], F_SETFL, 0); + fcntl(err[0], F_SETFL, 0); + } + if (communication & Stdout) { delete outnot; - while(childOutput(out[0])> 0 ) + while(childOutput(out[0])> 0 ) ; } if (communication & Stderr) { delete errnot; - while(childError(err[0]) > 0) + while(childError(err[0]) > 0) ; } + if (communication & Stdin) close(in[1]); if (communication & Stdout) @@ -659,8 +710,14 @@ input_data = 0; if (run_mode == Block) { - waitpid(pid, &status, 0); - processHasExited(status); + commClose(); + + // Its possible that the child's exit was caught by the SIGCHLD handler + // which will have set status for us. + if (waitpid(pid, &status, 0) != -1) this->status = status; + + runs = FALSE; + emit processExited(this); } } return TRUE; --- kprocctrl.cpp.orig Tue Jul 6 20:39:14 1999 +++ kprocctrl.cpp Tue Jul 6 21:49:07 1999 @@ -123,7 +123,15 @@ while (0L != proc) { if (proc->pid == pid) { - proc->processHasExited(status); + // process has exited, so do emit the respective events + if (proc->run_mode == KProcess::Block) { + // If the reads are done blocking then set the status in proc but do + // nothing else because KProcess will perform the other actions of + // processHasExited. + proc->status = status; + } else { + proc->processHasExited(status); + } } proc = processList->next(); } --------------F69FD8AA4FD2EC6A8348405B--