[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Please look at: [Fwd: Bug#1513: KProcess deadlock]
From:       Harri Porten <porten () tu-harburg ! de>
Date:       1999-07-07 18:28:35
[Download RAW message or body]

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

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 <smhp2803@hp00.rz.tu-harburg.de>; 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 <porten@tu-harburg.de>; 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 <alex@hayward.u-net.com>, 1513@bugs.kde.org
Resent-From: Alex Hayward <alex@hayward.u-net.com>
Resent-To: kde-bugs-dist@alpha.tat.physik.uni-tuebingen.de
Resent-CC: Kalle Dalheimer <kalle@kde.org>
Resent-Date: Wed, 07 Jul 1999 13:03:01 GMT
Resent-Message-ID: <handler.1513.B1513.93135239915813@bugs.kde.org>
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 <alex@hayward.u-net.com>
To: Harri Porten <porten@tu-harburg.de>
cc: 1513@bugs.kde.org
In-Reply-To: <377FA7D6.9B9EEA2A@tu-harburg.de>
Message-ID: <Pine.BSF.4.05.9907062125001.12766-100000@hayward.u-net.com>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Old-Return-Path: <bugs@max.tat.physik.uni-tuebingen.de>
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: <kde-bugs-dist@max.tat.physik.uni-tuebingen.de> 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();
   }



[prev in list] [next in list] [prev in thread] [next in thread]