GNU bug report logs - #15228
[PATCH] Close output port of I/O pipes

Previous Next

To reply to this bug, email your comments to 15228 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Josep Portella Florit <jpf <at> primfilat.com>
To: bug-guile <at> gnu.org
Subject: [PATCH] Close output port of I/O pipes
Date: Sat, 31 Aug 2013 10:29:57 +0200

There is a missing feature for pipes created with mode OPEN_BOTH:

(use-modules (ice-9 popen))
(use-modules (rnrs io ports))

(let ((p (open-pipe "md5sum" OPEN_BOTH)))
  (put-string p "hello")
  (let ((x (get-string-all p)))
    (close-pipe p)
    x))

This code deadlocks in get-string-all because md5sum, like other
filters, keeps waiting for input until the pipe's output port is
closed.

The output port can't be closed without closing the input port too,
because an I/O pipe is a soft port that doesn't store the 2 ports
returned by open-process, but a thunk which closes both ports.

This is now possible with the new procedure close-pipe-output:

(let ((p (open-pipe "md5sum" OPEN_BOTH)))
  (put-string p "hello")
  (close-pipe-output p)
  (let ((x (get-string-all p)))
    (close-pipe p)
    x))
;; => "5d41402abc4b2a76b9719d911017c592  -\n"

The intention is to make a backwards compatible and minimal change
that makes it possible to write to and read from pipes for filters
like md5sum without temporary files.

Changes involved:

* module/ice-9/popen.scm: Define a weak hash-table for mapping I/O pipes to
  their output ports, change make-rw-port to use it, define the
  close-pipe-output procedure and export it.

* doc/ref/posix.texi: Add documentation for close-pipe-output.

On garbage collection the new hash-table is updated as expected:

scheme@(ice-9 popen)> rw/w-table
$3 = #<weak-key-hash-table 8b8a930 0/31>
scheme@(ice-9 popen)> (define p (open-pipe "md5sum" OPEN_BOTH))
scheme@(ice-9 popen)> rw/w-table
$4 = #<weak-key-hash-table 8b8a930 1/31>
scheme@(ice-9 popen)> (set! p #f)
scheme@(ice-9 popen)> (gc)
scheme@(ice-9 popen)> rw/w-table
$5 = #<weak-key-hash-table 8b8a930 0/31>

Maybe there is a better name for the new procedure.
---
 doc/ref/posix.texi     |    6 ++++++
 module/ice-9/popen.scm |   39 +++++++++++++++++++++++++++++----------
 2 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/doc/ref/posix.texi b/doc/ref/posix.texi
index b3a6a04..f0c6ca1 100644
--- a/doc/ref/posix.texi
+++ b/doc/ref/posix.texi
@@ -2312,6 +2312,12 @@ terminate, and return the wait status code.  The status is as per
 (@pxref{Processes})
 @end deffn
 
+@deffn {Scheme Procedure} close-pipe-output port
+Close the output port of a pipe created by @code{open-pipe} with
+mode @code{OPEN_BOTH}, and leave the input port open.  Return `#t' if
+the port is closed successfully or `#f' if it was already closed.
+@end deffn
+
 @sp 1
 @code{waitpid WAIT_ANY} should not be used when pipes are open, since
 it can reap a pipe's child process, causing an error from a subsequent
diff --git a/module/ice-9/popen.scm b/module/ice-9/popen.scm
index 7d0549e..2b014c5 100644
--- a/module/ice-9/popen.scm
+++ b/module/ice-9/popen.scm
@@ -18,22 +18,32 @@
 ;;;; 
 
 (define-module (ice-9 popen)
-  :export (port/pid-table open-pipe* open-pipe close-pipe open-input-pipe
-	   open-output-pipe open-input-output-pipe))
+  :export (port/pid-table open-pipe* open-pipe close-pipe close-pipe-output
+           open-input-pipe open-output-pipe open-input-output-pipe))
 
 (eval-when (load eval compile)
   (load-extension (string-append "libguile-" (effective-version))
                   "scm_init_popen"))
 
+;; a weak hash-table to store the write port of read-write pipes
+;; just to be able to retrieve it in close-pipe-output.
+(define rw/w-table (make-weak-key-hash-table 31))
+
 (define (make-rw-port read-port write-port)
-  (make-soft-port
-   (vector
-    (lambda (c) (write-char c write-port))
-    (lambda (s) (display s write-port))
-    (lambda () (force-output write-port))
-    (lambda () (read-char read-port))
-    (lambda () (close-port read-port) (close-port write-port)))
-   "r+"))
+  (letrec ((port (make-soft-port
+                  (vector
+                   (lambda (c) (write-char c write-port))
+                   (lambda (s) (display s write-port))
+                   (lambda () (force-output write-port))
+                   (lambda () (read-char read-port))
+                   (lambda ()
+                     (hashq-remove! rw/w-table port)
+                     (close-port read-port)
+                     (or (port-closed? write-port)
+                         (close-port write-port))))
+                  "r+")))
+    (hashq-set! rw/w-table port write-port)
+    port))
 
 ;; a guardian to ensure the cleanup is done correctly when
 ;; an open pipe is gc'd or a close-port is used.
@@ -106,6 +116,15 @@ information on how to interpret this value."
         (error "close-pipe: pipe not in table"))
     (close-process (cons p pid))))
 
+(define (close-pipe-output pipe)
+  "Closes the output port of a pipe created by @code{open-pipe} with
+mode @code{OPEN_BOTH}, and leaves the input port open.  Returns `#t' if
+it successfully closes the port or `#f' if it was already closed."
+  (let ((port (hashq-ref rw/w-table pipe)))
+    (unless port
+      (error "close-pipe-output: pipe not in table"))
+    (close-port port)))
+
 (define reap-pipes
   (lambda ()
     (let loop ((p (pipe-guardian)))
-- 
1.7.9.5

Message #8 received at 15228 <at> debbugs.gnu.org (full text, mbox):

From: Andy Wingo <wingo <at> pobox.com>
To: Josep Portella Florit <jpf <at> primfilat.com>
Cc: 15228 <at> debbugs.gnu.org, guile-devel <at> gnu.org
Subject: Re: bug#15228: [PATCH] Close output port of I/O pipes
Date: Tue, 21 Jun 2016 12:47:38 +0200

Hi :)

I dunno how much we should push this "processes are a single port"
abstraction.  In many ways for OPEN_BOTH pipes it's easier to deal with
an input and an output port and a PID instead of the pipe abstraction.
WDYT?  We could just expose `open-process' from (ice-9 popen) to start
with.  It would be good to allow other fd's or ports to map to the child
as well, e.g. stderr or any particular port; but I don't know what
interface we should expose.

Thoughts?

Andy

On Sat 31 Aug 2013 10:29, Josep Portella Florit <jpf <at> primfilat.com> writes:

> There is a missing feature for pipes created with mode OPEN_BOTH:
>
> (use-modules (ice-9 popen))
> (use-modules (rnrs io ports))
>
> (let ((p (open-pipe "md5sum" OPEN_BOTH)))
>   (put-string p "hello")
>   (let ((x (get-string-all p)))
>     (close-pipe p)
>     x))
>
> This code deadlocks in get-string-all because md5sum, like other
> filters, keeps waiting for input until the pipe's output port is
> closed.
>
> The output port can't be closed without closing the input port too,
> because an I/O pipe is a soft port that doesn't store the 2 ports
> returned by open-process, but a thunk which closes both ports.
>
> This is now possible with the new procedure close-pipe-output:
>
> (let ((p (open-pipe "md5sum" OPEN_BOTH)))
>   (put-string p "hello")
>   (close-pipe-output p)
>   (let ((x (get-string-all p)))
>     (close-pipe p)
>     x))
> ;; => "5d41402abc4b2a76b9719d911017c592  -\n"
>
> The intention is to make a backwards compatible and minimal change
> that makes it possible to write to and read from pipes for filters
> like md5sum without temporary files.
>
> Changes involved:
>
> * module/ice-9/popen.scm: Define a weak hash-table for mapping I/O pipes to
>   their output ports, change make-rw-port to use it, define the
>   close-pipe-output procedure and export it.
>
> * doc/ref/posix.texi: Add documentation for close-pipe-output.
>
> On garbage collection the new hash-table is updated as expected:
>
> scheme@(ice-9 popen)> rw/w-table
> $3 = #<weak-key-hash-table 8b8a930 0/31>
> scheme@(ice-9 popen)> (define p (open-pipe "md5sum" OPEN_BOTH))
> scheme@(ice-9 popen)> rw/w-table
> $4 = #<weak-key-hash-table 8b8a930 1/31>
> scheme@(ice-9 popen)> (set! p #f)
> scheme@(ice-9 popen)> (gc)
> scheme@(ice-9 popen)> rw/w-table
> $5 = #<weak-key-hash-table 8b8a930 0/31>
>
> Maybe there is a better name for the new procedure.
> ---
>  doc/ref/posix.texi     |    6 ++++++
>  module/ice-9/popen.scm |   39 +++++++++++++++++++++++++++++----------
>  2 files changed, 35 insertions(+), 10 deletions(-)
>
> diff --git a/doc/ref/posix.texi b/doc/ref/posix.texi
> index b3a6a04..f0c6ca1 100644
> --- a/doc/ref/posix.texi
> +++ b/doc/ref/posix.texi
> @@ -2312,6 +2312,12 @@ terminate, and return the wait status code.  The status is as per
>  (@pxref{Processes})
>  @end deffn
>  
> +@deffn {Scheme Procedure} close-pipe-output port
> +Close the output port of a pipe created by @code{open-pipe} with
> +mode @code{OPEN_BOTH}, and leave the input port open.  Return `#t' if
> +the port is closed successfully or `#f' if it was already closed.
> +@end deffn
> +
>  @sp 1
>  @code{waitpid WAIT_ANY} should not be used when pipes are open, since
>  it can reap a pipe's child process, causing an error from a subsequent
> diff --git a/module/ice-9/popen.scm b/module/ice-9/popen.scm
> index 7d0549e..2b014c5 100644
> --- a/module/ice-9/popen.scm
> +++ b/module/ice-9/popen.scm
> @@ -18,22 +18,32 @@
>  ;;;; 
>  
>  (define-module (ice-9 popen)
> -  :export (port/pid-table open-pipe* open-pipe close-pipe open-input-pipe
> -	   open-output-pipe open-input-output-pipe))
> +  :export (port/pid-table open-pipe* open-pipe close-pipe close-pipe-output
> +           open-input-pipe open-output-pipe open-input-output-pipe))
>  
>  (eval-when (load eval compile)
>    (load-extension (string-append "libguile-" (effective-version))
>                    "scm_init_popen"))
>  
> +;; a weak hash-table to store the write port of read-write pipes
> +;; just to be able to retrieve it in close-pipe-output.
> +(define rw/w-table (make-weak-key-hash-table 31))
> +
>  (define (make-rw-port read-port write-port)
> -  (make-soft-port
> -   (vector
> -    (lambda (c) (write-char c write-port))
> -    (lambda (s) (display s write-port))
> -    (lambda () (force-output write-port))
> -    (lambda () (read-char read-port))
> -    (lambda () (close-port read-port) (close-port write-port)))
> -   "r+"))
> +  (letrec ((port (make-soft-port
> +                  (vector
> +                   (lambda (c) (write-char c write-port))
> +                   (lambda (s) (display s write-port))
> +                   (lambda () (force-output write-port))
> +                   (lambda () (read-char read-port))
> +                   (lambda ()
> +                     (hashq-remove! rw/w-table port)
> +                     (close-port read-port)
> +                     (or (port-closed? write-port)
> +                         (close-port write-port))))
> +                  "r+")))
> +    (hashq-set! rw/w-table port write-port)
> +    port))
>  
>  ;; a guardian to ensure the cleanup is done correctly when
>  ;; an open pipe is gc'd or a close-port is used.
> @@ -106,6 +116,15 @@ information on how to interpret this value."
>          (error "close-pipe: pipe not in table"))
>      (close-process (cons p pid))))
>  
> +(define (close-pipe-output pipe)
> +  "Closes the output port of a pipe created by @code{open-pipe} with
> +mode @code{OPEN_BOTH}, and leaves the input port open.  Returns `#t' if
> +it successfully closes the port or `#f' if it was already closed."
> +  (let ((port (hashq-ref rw/w-table pipe)))
> +    (unless port
> +      (error "close-pipe-output: pipe not in table"))
> +    (close-port port)))
> +
>  (define reap-pipes
>    (lambda ()
>      (let loop ((p (pipe-guardian)))

Message #11 received at 15228 <at> debbugs.gnu.org (full text, mbox):

From: Andy Wingo <wingo <at> pobox.com>
To: 15228 <at> debbugs.gnu.org
Subject: Re: bug#15228: [PATCH] Close output port of I/O pipes
Date: Mon, 27 Jun 2016 10:05:06 +0200

[bouncing this back to debbugs]

On Sat 25 Jun 2016 17:49, Andy Wingo <wingo <at> pobox.com> writes:

> On Sat 25 Jun 2016 15:51, Josep Portella Florit <jpf <at> primfilat.com> writes:
>
>>> I dunno how much we should push this "processes are a single port"
>>> abstraction.  In many ways for OPEN_BOTH pipes it's easier to deal with
>>> an input and an output port and a PID instead of the pipe abstraction.
>>> WDYT?  We could just expose `open-process' from (ice-9 popen) to start
>>> with.  It would be good to allow other fd's or ports to map to the child
>>> as well, e.g. stderr or any particular port; but I don't know what
>>> interface we should expose.
>>
>> Since patching was inconvenient for me, I eventually used:
>>
>> (use-modules ((ice-9 popen) #:select (open-process)))
>>
>> Which works even though `open-process` is not exported.
>
> Note that this behavior of #:select is a bug.  We won't remove it in
> stable-2.0 but we have removed it in master.
>
>> For me, exporting `open-process` and documenting it would be enough.
>
> Fine with me, many people have asked for this at this point.  I guess
> that's the next step for this bug.
>
>> I also like the Racket interface to processes:
>> <https://docs.racket-lang.org/reference/subprocess.html>
>> (I've mostly used the `process` procedure.)
>
> Duly noted!  The more we steal from Racket, the better Guile will be :)
>
> Andy

Message #14 received at 15228 <at> debbugs.gnu.org (full text, mbox):

From: Amirouche Boubekki <amirouche <at> hypermove.net>
To: 15228 <at> debbugs.gnu.org
Subject: making open-process public
Date: Sat, 03 Sep 2016 09:33:32 +0200

Wingo wrote:

> We could just expose `open-process' from (ice-9 popen) to start with.

AFAIK, that's what Mark wants.

Here is an example use of `open-process' to wrap `html2text':


   (use-modules (ice-9 popen))

   (define open-process (@@ (ice-9 popen) open-process))

   (define (html2text string)
     (with-error-to-file "/dev/null"
       (lambda ()
         (call-with-values (lambda () (open-process OPEN_BOTH 
"html2text"))
           (lambda (read-port write-port pid)
             (display string write-port)
             (close-port write-port)
             (let ((str (read-string read-port)))
               (close-port read-port)
               (waitpid pid)
               str))))))

IIUC to achieve this goal, I need to make `open-process' public
in `ice-9 popen` module and add documentation for it?

Is that correct?

Message #17 received at 15228 <at> debbugs.gnu.org (full text, mbox):

From: Andy Wingo <wingo <at> pobox.com>
To: Amirouche Boubekki <amirouche <at> hypermove.net>
Cc: 15228 <at> debbugs.gnu.org
Subject: Re: bug#15228: making open-process public
Date: Tue, 28 Feb 2017 10:58:18 +0100

On Sat 03 Sep 2016 09:33, Amirouche Boubekki <amirouche <at> hypermove.net> writes:

>> We could just expose `open-process' from (ice-9 popen) to start with.
>
> Here is an example use of `open-process' to wrap `html2text':
>
>
>    (use-modules (ice-9 popen))
>
>    (define open-process (@@ (ice-9 popen) open-process))
>
>    (define (html2text string)
>      (with-error-to-file "/dev/null"
>        (lambda ()
>          (call-with-values (lambda () (open-process OPEN_BOTH
> "html2text"))
>            (lambda (read-port write-port pid)
>              (display string write-port)
>              (close-port write-port)
>              (let ((str (read-string read-port)))
>                (close-port read-port)
>                (waitpid pid)
>                str))))))
>
> IIUC to achieve this goal, I need to make `open-process' public
> in `ice-9 popen` module and add documentation for it?
>
> Is that correct?

Yep!  Let's just do this.  Would you mind sending a patch.

Andy

Message #20 received at 15228 <at> debbugs.gnu.org (full text, mbox):

From: Amirouche <amirouche <at> hypermove.net>
To: 15228 <at> debbugs.gnu.org
Subject: Re: bug#15228: making open-process public
Date: Sun, 5 Mar 2017 21:41:56 +0100

[Message part 1 (text/plain, inline)]

Sorry I have an issue with my mail I can't find the mail where you asked 
for a patch.

Attached to this mail my very first patch.


Le 03/09/2016 à 09:33, Amirouche Boubekki a écrit :
> Wingo wrote:
>
>> We could just expose `open-process' from (ice-9 popen) to start with.
>
> AFAIK, that's what Mark wants.
>
> Here is an example use of `open-process' to wrap `html2text':
>
>
>    (use-modules (ice-9 popen))
>
>    (define open-process (@@ (ice-9 popen) open-process))
>
>    (define (html2text string)
>      (with-error-to-file "/dev/null"
>        (lambda ()
>          (call-with-values (lambda () (open-process OPEN_BOTH 
> "html2text"))
>            (lambda (read-port write-port pid)
>              (display string write-port)
>              (close-port write-port)
>              (let ((str (read-string read-port)))
>                (close-port read-port)
>                (waitpid pid)
>                str))))))
>
> IIUC to achieve this goal, I need to make `open-process' public
> in `ice-9 popen` module and add documentation for it?
>
> Is that correct?
>
>
>

[0002-make-open-process-public-in-ice-9-popen.patch (text/x-patch, attachment)]

[0001-whitespace-cleanup.patch (text/x-patch, attachment)]

Message #23 received at 15228 <at> debbugs.gnu.org (full text, mbox):

From: Adriano Peluso <randomlooser <at> riseup.net>
To: 15228 <at> debbugs.gnu.org
Subject: can this be closed ?
Date: Wed, 02 Jun 2021 07:45:10 +0200

Is this bug still current ?

Can this be closed ?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.