"no duplicate" in `popen.test'

"no duplicate" in `popen.test'

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

Hello,

That "no duplicate" test in `popen.test' leaves a zombie behind it [0].
The fix would be to `waitpid' the process created by `open-input-pipe'
(see attached patch), but that makes it hang, waiting for "sleep 999" to
complete.

I'm not sure whether it's an indication that the bug was caught, or
rather an indication that the test is broken, especially since I don't
fully understand the bug that it's trying to catch.

I found these two relevant threads:

  http://article.gmane.org/gmane.lisp.guile.devel/6010 [1]
  http://article.gmane.org/gmane.lisp.guile.bugs/3560

Apparently, Kevin wasn't too confident in the test but decided to leave
it in the lack of a better replacement.

I didn't find any message explaining why the test came into existence.

I would appreciate feedback on this since it's tempting to remove it
altogether for all the harm it's done.  ;-)

Thanks,
Ludovic.

[0] FWIW, this breaks NixOS builds with test-suite enabled since
    `nix-worker', the deamon that builds things, waits for all child
    processes.

[1] It's this very test that led Debian to build 1.8
    `--without-threads'!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: The patch --]
[-- Type: text/x-patch, Size: 838 bytes --]

--- guile/test-suite/tests/popen.test	2006-08-25 03:21:39.000000000 +0200
+++ guile/test-suite/tests/popen.test	2008-03-14 10:20:23.000000000 +0100
@@ -1,6 +1,6 @@
 ;;;; popen.test --- exercise ice-9/popen.scm      -*- scheme -*-
 ;;;;
-;;;; Copyright 2003, 2006 Free Software Foundation, Inc.
+;;;; Copyright 2003, 2006, 2008 Free Software Foundation, Inc.
 ;;;;
 ;;;; This library is free software; you can redistribute it and/or
 ;;;; modify it under the terms of the GNU Lesser General Public
@@ -86,7 +86,9 @@
       (close-port (cdr pair))   ;; write side
       (and (char? (read-char (car pair))) ;; wait for child to do its thing
 	   (char-ready? port)
-	   (eof-object? (read-char port))))))
+	   (let ((pass? (eof-object? (read-char port))))
+             (close-pipe port)
+             pass?)))))
 
 ;;
 ;; open-output-pipe

Re: "no duplicate" in `popen.test'

[Neil Jerram]

[-- Attachment #1: Type: text/plain, Size: 1497 bytes --]

ludo@gnu.org (Ludovic Courtès) writes:

> Hello,
>
> That "no duplicate" test in `popen.test' leaves a zombie behind it [0].
> The fix would be to `waitpid' the process created by `open-input-pipe'
> (see attached patch), but that makes it hang, waiting for "sleep 999" to
> complete.

I've attached an alternative possible solution, using feedback from
the parent to the child to avoid needing the long sleep.

Unfortunately, though, I didn't manage to observe the zombie process
with the test as it was before.  (How do I do this on GNU/Linux?)  So
I don't really know whether this is a significant improvement.

> I'm not sure whether it's an indication that the bug was caught, or
> rather an indication that the test is broken, especially since I don't
> fully understand the bug that it's trying to catch.

If waitpid fixes it, doesn't that point to the test being broken?

> I would appreciate feedback on this since it's tempting to remove it
> altogether for all the harm it's done.  ;-)

It seems a worthwhile test to me.  I'd rather work on a tricky
test like this, than on a report of a problem in the context of a
whole application (but which eventually boiled down to the same
thing).

I'm also interested in the forking/threads issue (which is now fixed,
if I've understood the cited thread correctly), and wouldn't want to
remove a test that covered that, unless we can devise a more specific
test for just that issue.

Regards,
        Neil


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: popen.diff --]
[-- Type: text/x-diff, Size: 1105 bytes --]

Index: test-suite/tests/popen.test
===================================================================
RCS file: /cvsroot/guile/guile/guile-core/test-suite/tests/popen.test,v
retrieving revision 1.3.2.2
diff -u -r1.3.2.2 popen.test
--- test-suite/tests/popen.test	25 Aug 2006 01:21:39 -0000	1.3.2.2
+++ test-suite/tests/popen.test	17 Mar 2008 22:54:07 -0000
@@ -81,12 +81,15 @@
     (let* ((pair (pipe))
 	   (port (with-error-to-port (cdr pair)
 		   (lambda ()
-		     (open-input-pipe
-		      "exec 1>/dev/null; echo closed 1>&2; exec 2>/dev/null; sleep 999")))))
+		     (open-input-output-pipe
+		      "exec 1>/dev/null; echo closed 1>&2; exec 2>/dev/null; read")))))
       (close-port (cdr pair))   ;; write side
-      (and (char? (read-char (car pair))) ;; wait for child to do its thing
-	   (char-ready? port)
-	   (eof-object? (read-char port))))))
+      (let ((result (and (char? (read-char (car pair))) ;; wait for child to do its thing
+			 (char-ready? port)
+			 (eof-object? (read-char port)))))
+	(display "hello!\n" port)
+	(close-pipe port)
+	result))))
 
 ;;
 ;; open-output-pipe

Re: "no duplicate" in `popen.test'

[Ludovic Courtès]

Hi Neil,

Neil Jerram <neil@ossau.uklinux.net> writes:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Hello,
>>
>> That "no duplicate" test in `popen.test' leaves a zombie behind it [0].
>> The fix would be to `waitpid' the process created by `open-input-pipe'
>> (see attached patch), but that makes it hang, waiting for "sleep 999" to
>> complete.
>
> I've attached an alternative possible solution, using feedback from
> the parent to the child to avoid needing the long sleep.

Good idea.

It apparently fixes the problem.  I think we also need that change in
the second "no duplicate" test, don't we?

> Unfortunately, though, I didn't manage to observe the zombie process
> with the test as it was before.  (How do I do this on GNU/Linux?)  So
> I don't really know whether this is a significant improvement.

You can just run "./check-guile && pstree -u $USER".  That shows whether
there are any processes left (whether zombie or not).  "ps" can then
tell whether these are zombies, when marked with a `Z'.

>> I'm not sure whether it's an indication that the bug was caught, or
>> rather an indication that the test is broken, especially since I don't
>> fully understand the bug that it's trying to catch.
>
> If waitpid fixes it, doesn't that point to the test being broken?

Hmm, I don't know, why?

Thanks,
Ludovic.

Re: "no duplicate" in `popen.test'

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

Hi,

Neil Jerram <neil@ossau.uklinux.net> writes:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> That "no duplicate" test in `popen.test' leaves a zombie behind it [0].
>> The fix would be to `waitpid' the process created by `open-input-pipe'
>> (see attached patch), but that makes it hang, waiting for "sleep 999" to
>> complete.
>
> I've attached an alternative possible solution, using feedback from
> the parent to the child to avoid needing the long sleep.

Can you apply this patch?

I have a slightly modified version in NixOS, which is your patch + the
using `read' in the second "no duplicate" test.

Thanks,
Ludovic.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: The patch --]
[-- Type: text/x-patch, Size: 1877 bytes --]

Index: guile/test-suite/tests/popen.test
===================================================================
RCS file: /sources/guile/guile/guile-core/guile/test-suite/tests/popen.test,v
retrieving revision 1.3.2.2
diff -u -r1.3.2.2 popen.test
--- guile/test-suite/tests/popen.test	25 Aug 2006 01:21:39 -0000	1.3.2.2
+++ guile/test-suite/tests/popen.test	18 Mar 2008 20:18:08 -0000
@@ -1,6 +1,6 @@
 ;;;; popen.test --- exercise ice-9/popen.scm      -*- scheme -*-
 ;;;;
-;;;; Copyright 2003, 2006 Free Software Foundation, Inc.
+;;;; Copyright 2003, 2006, 2008 Free Software Foundation, Inc.
 ;;;;
 ;;;; This library is free software; you can redistribute it and/or
 ;;;; modify it under the terms of the GNU Lesser General Public
@@ -81,12 +81,15 @@
     (let* ((pair (pipe))
 	   (port (with-error-to-port (cdr pair)
 		   (lambda ()
-		     (open-input-pipe
-		      "exec 1>/dev/null; echo closed 1>&2; exec 2>/dev/null; sleep 999")))))
+		     (open-input-output-pipe
+		      "exec 1>/dev/null; echo closed 1>&2; exec 2>/dev/null; read")))))
       (close-port (cdr pair))   ;; write side
-      (and (char? (read-char (car pair))) ;; wait for child to do its thing
-	   (char-ready? port)
-	   (eof-object? (read-char port))))))
+      (let ((result (and (char? (read-char (car pair))) ;; wait for child to do its thing
+			 (char-ready? port)
+			 (eof-object? (read-char port)))))
+	(display "hello!\n" port)
+	(close-pipe port)
+	result))))
 
 ;;
 ;; open-output-pipe
@@ -132,7 +135,7 @@
 	      (port (with-error-to-port (cdr pair)
 		      (lambda ()
 			(open-output-pipe
-			 "exec 0</dev/null; echo closed 1>&2; exec 2>/dev/null; sleep 999")))))
+			 "exec 0</dev/null; echo closed 1>&2; exec 2>/dev/null; read")))))
 	 (close-port (cdr pair))   ;; write side
 	 (and (char? (read-char (car pair))) ;; wait for child to do its thing
 	      (catch 'system-error

Re: "no duplicate" in `popen.test'

[Neil Jerram]

2008/5/19 Ludovic Courtès <ludo@gnu.org>:
> Hi,
>
> Neil Jerram <neil@ossau.uklinux.net> writes:
>
>> ludo@gnu.org (Ludovic Courtès) writes:
>>
>>> That "no duplicate" test in `popen.test' leaves a zombie behind it [0].
>>> The fix would be to `waitpid' the process created by `open-input-pipe'
>>> (see attached patch), but that makes it hang, waiting for "sleep 999" to
>>> complete.
>>
>> I've attached an alternative possible solution, using feedback from
>> the parent to the child to avoid needing the long sleep.
>
> Can you apply this patch?

Well...  Looking at the patch again, I thought "but it can't be right
to do a display to port, when port comes from open-input-pipe".  And
then I tried running the test again, with the patch in place, and:

ERROR: open-input-pipe: no duplicate - arguments: ((wrong-type-arg
"display" "Wrong type argument in position ~A: ~S" (2 #<input: #{read\
pipe}# 13>) (#<input:
#{read\ pipe}# 13>)))

I'm not currently sure why I missed that before, but I'm working on a
better patch.

    Neil

Re: "no duplicate" in `popen.test'

[Neil Jerram]

[-- Attachment #1: Type: text/plain, Size: 810 bytes --]

"Neil Jerram" <neiljerram@googlemail.com> writes:

> 2008/5/19 Ludovic Courtès <ludo@gnu.org>:
>>
>> Can you apply this patch?
>
> Well...  Looking at the patch again, I thought "but it can't be right
> to do a display to port, when port comes from open-input-pipe".  And
> then I tried running the test again, with the patch in place, and:
>
> ERROR: open-input-pipe: no duplicate - arguments: ((wrong-type-arg
> "display" "Wrong type argument in position ~A: ~S" (2 #<input: #{read\
> pipe}# 13>) (#<input:
> #{read\ pipe}# 13>)))
>
> I'm not currently sure why I missed that before, but I'm working on a
> better patch.

I finally got back to this.  I'm pretty sure the attached patch is now
correct, so will commit in a couple of days unless anyone objects.

Regards,
        Neil


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Avoid-no-duplicate-popen-tests-leaving-zombie-proc.patch --]
[-- Type: text/x-diff, Size: 5226 bytes --]

From f7329523787596bb7ea5c00f05c60d8649e16eab Mon Sep 17 00:00:00 2001
From: Neil Jerram <neil@ossau.uklinux.net>
Date: Sun, 7 Sep 2008 16:29:05 +0100
Subject: [PATCH] Avoid "no duplicate" popen tests leaving zombie processes

On the one hand we want the child process in these tests to exit.  On
the other, we don't want it to exit before the parent Guile code has
tested the relevant condition (EOF in the first test, broken pipe in
the second) - because these conditions would obviously be true if the
child had already exited, and that's not what we're trying to test
here.  We're trying to test getting EOF and broken pipe while the
child process is still alive.

* test-suite/tests/popen.test (open-input-pipe:no duplicate): Add
  another pipe from parent to child, so that the child can finish by
  reading from this.  Then the parent controls the child lifetime by
  writing to this pipe.

  test-suite/tests/popen.test (open-output-pipe:no duplicate): Add
  another pipe from child to parent, and have the child finish by
  endlessly writing into this.  Then the parent controls the child
  lifetime by closing its end of the pipe, causing a broken pipe in
  the child.
---
 test-suite/tests/popen.test |   86 +++++++++++++++++++++++++++++--------------
 1 files changed, 58 insertions(+), 28 deletions(-)

diff --git a/test-suite/tests/popen.test b/test-suite/tests/popen.test
index 1dd2bc7..c4b7477 100644
--- a/test-suite/tests/popen.test
+++ b/test-suite/tests/popen.test
@@ -73,20 +73,39 @@
 	      (open-input-pipe "echo hello"))))))
     #t)
   
+  (pass-if "open-input-pipe process gets (current-input-port) as stdin"
+    (let* ((p2c (pipe))
+           (port (with-input-from-port (car p2c)
+                   (lambda ()
+                     (open-input-pipe "read && echo $REPLY")))))
+      (display "hello\n" (cdr p2c))
+      (force-output (cdr p2c))
+      (let ((result (eq? (read port) 'hello)))
+	(close-port (cdr p2c))
+	(close-pipe port)
+	result)))
+
   ;; After the child closes stdout (which it indicates here by writing
   ;; "closed" to stderr), the parent should see eof.  In Guile 1.6.4 and
   ;; earlier a duplicate of stdout existed in the child, meaning eof was not
   ;; seen.
   (pass-if "no duplicate"
-    (let* ((pair (pipe))
-	   (port (with-error-to-port (cdr pair)
+    (let* ((c2p (pipe))
+	   (p2c (pipe))
+	   (port (with-error-to-port (cdr c2p)
 		   (lambda ()
-		     (open-input-pipe
-		      "exec 1>/dev/null; echo closed 1>&2; exec 2>/dev/null; sleep 999")))))
-      (close-port (cdr pair))   ;; write side
-      (and (char? (read-char (car pair))) ;; wait for child to do its thing
-	   (char-ready? port)
-	   (eof-object? (read-char port))))))
+		     (with-input-from-port (car p2c)
+		       (lambda ()
+			 (open-input-pipe
+			  "exec 1>/dev/null; echo closed 1>&2; exec 2>/dev/null; read")))))))
+      (close-port (cdr c2p))   ;; write side
+      (let ((result (eof-object? (read-char port))))
+	(display "hello!\n" (cdr p2c))
+	(force-output (cdr p2c))
+	(close-pipe port)
+	result)))
+
+  )
 
 ;;
 ;; open-output-pipe
@@ -121,27 +140,38 @@
     #t)
   
   ;; After the child closes stdin (which it indicates here by writing
-  ;; "closed" to stderr), the parent should see a broken pipe.  We setup to
-  ;; see this as EPIPE (rather than SIGPIPE).  In Guile 1.6.4 and earlier a
-  ;; duplicate of stdin existed in the child, preventing the broken pipe
-  ;; occurring.
+  ;; "closed" to stderr), the parent should see a broken pipe.  We
+  ;; setup to see this as EPIPE (rather than SIGPIPE).  In Guile 1.6.4
+  ;; and earlier a duplicate of stdin existed in the child, preventing
+  ;; the broken pipe occurring.  Note that `with-epipe' must apply
+  ;; only to the parent and not to the child process; we rely on the
+  ;; child getting SIGPIPE, to terminate it (and avoid leaving a
+  ;; zombie).
   (pass-if "no duplicate"
-    (with-epipe
-     (lambda ()
-       (let* ((pair (pipe))
-	      (port (with-error-to-port (cdr pair)
-		      (lambda ()
-			(open-output-pipe
-			 "exec 0</dev/null; echo closed 1>&2; exec 2>/dev/null; sleep 999")))))
-	 (close-port (cdr pair))   ;; write side
-	 (and (char? (read-char (car pair))) ;; wait for child to do its thing
-	      (catch 'system-error
-		(lambda ()
-		  (write-char #\x port)
-		  (force-output port)
-		  #f)
-		(lambda (key name fmt args errno-list)
-		  (= (car errno-list) EPIPE)))))))))
+    (let* ((c2p (pipe))
+	   (port (with-error-to-port (cdr c2p)
+		   (lambda ()
+		     (open-output-pipe
+		      "exec 0</dev/null; while true; do echo closed 1>&2; done")))))
+      (close-port (cdr c2p))   ;; write side
+      (with-epipe
+       (lambda ()
+	 (let ((result
+		(and (char? (read-char (car c2p))) ;; wait for child to do its thing
+		     (catch 'system-error
+			    (lambda ()
+			      (write-char #\x port)
+			      (force-output port)
+			      #f)
+			    (lambda (key name fmt args errno-list)
+			      (= (car errno-list) EPIPE))))))
+	   ;; Now close our reading end of the pipe.  This should give
+	   ;; the child a broken pipe and so allow it to exit.
+	   (close-port (car c2p))
+	   (close-pipe port)
+	   result)))))
+
+  )
 
 ;;
 ;; close-pipe
-- 
1.5.6.5

Re: "no duplicate" in `popen.test'

[Ludovic Courtès]

Hi Neil,

Neil Jerram <neil@ossau.uklinux.net> writes:

> I finally got back to this.  I'm pretty sure the attached patch is now
> correct, so will commit in a couple of days unless anyone objects.

Thanks for working on it!  Please go ahead.

> * test-suite/tests/popen.test (open-input-pipe:no duplicate): Add
>   another pipe from parent to child, so that the child can finish by
>   reading from this.  Then the parent controls the child lifetime by
>   writing to this pipe.
>
>   test-suite/tests/popen.test (open-output-pipe:no duplicate): Add
>   another pipe from child to parent, and have the child finish by
>   endlessly writing into this.  Then the parent controls the child
>   lifetime by closing its end of the pipe, causing a broken pipe in
>   the child.

One nitpick: this entry doesn't completely follow the GNU ChangeLog
style.  Also, the rationale for the change belongs in comments in
`popen.test' rather than in the commit log.

Thanks,
Ludo'.

Re: "no duplicate" in `popen.test'

[Neil Jerram]

ludo@gnu.org (Ludovic Courtès) writes:

>> * test-suite/tests/popen.test (open-input-pipe:no duplicate): Add
>>   another pipe from parent to child, so that the child can finish by
>>   reading from this.  Then the parent controls the child lifetime by
>>   writing to this pipe.
>>
>>   test-suite/tests/popen.test (open-output-pipe:no duplicate): Add
>>   another pipe from child to parent, and have the child finish by
>>   endlessly writing into this.  Then the parent controls the child
>>   lifetime by closing its end of the pipe, causing a broken pipe in
>>   the child.
>
> One nitpick: this entry doesn't completely follow the GNU ChangeLog
> style.  Also, the rationale for the change belongs in comments in
> `popen.test' rather than in the commit log.

Ah, you caught me; I wrote that one by hand...  I'll fix it up.

Thanks,
        Neil