Make it possible again to read STDERR without calling `cat` #11919

Keno · 2015-06-28T19:10:13Z

Merge the Pipe() and Pipe(C_NULL) constructors having the former just
call the latter and delaying initialization to the place where it is
actually needed. Now passing an empty Pipe to spawn will once again
initialize it with the appropriate pipe end. Fixes #11824.

tkelman · 2015-06-28T19:16:36Z

base/exports.jl

@@ -1149,6 +1149,7 @@ export
    ntoh,
    open,
    pipe,
+    Pipe,


kinda confusing to export this when it's a rather different thing than the pipe function

I think once processes are readable/writable, this can just be pipe() (although @vtjnash wants that to return a pair). I think maybe the right design is instead of have another method that creates the other pipe end.

Maybe have a real API design discussion about this?

I think I'd be ok with calling this pipe() and adding pipepair() in the future if necessary. Names that differ only in case are really annoying.

So what does the pipe function do now then? Either connect two commands via a pipe or construct a pipe object?

Keno and I both suggested that at the beginning of this thread 17 days ago.

The problem with that is that pipe with more than one argument is a lazy constructor for a command object, while Pipe() creates a pipe object that run modifies.

I think it might just have to be Pipe and pipeline, though I sorta dread having to tell people they have to go from one character to 8 for this functionality.

Of course, I also suggested const 🚰= Pipe

Hmm. Maybe having pipe and Pipe isn't so bad :-\

One suggestion that I forgot to mention here was pipe and PipeEnd. I also considered Piping (as in to get data out of your pipeline, just add some Piping).

vtjnash · 2015-07-13T05:48:00Z

base/process.jl

@@ -311,7 +311,9 @@ macro setup_stdio()
        in,out,err = stdios
        if isa(stdios[1], Pipe)
            if stdios[1].handle == C_NULL


ref #10469 (comment)

reminder (this should be using the test isopen(stdios[1]), now that we have that field)

As per our discussion, the C_NULL here should be fine as this way you get an error in the use-after-close case whereas otherwise it would silently pass /dev/null.

tkelman · 2015-07-14T23:32:51Z

spawn test looks stuck on win32 and win64 appveyor

also ref https://github.com/JuliaLang/julia/pull/11919/files#r33425946

Keno · 2015-07-17T19:08:35Z

Renamed pipe to pipeline. I'm planning a whole documentation overhaul, but given that the doc transition has yet to happen, I haven't touched docs at all. Once that's done I'll do docs.

JeffBezanson · 2015-07-28T16:30:41Z

Hmm, still times out in CI.

Keno · 2015-07-28T16:31:21Z

Yeah, I'll take a look.

Merge the Pipe() and Pipe(C_NULL) constructors having the former just call the latter and delaying initialization to the place where it is actually needed. Now passing an empty Pipe to spawn will once again initialize it with the appropriate pipe end.

Keno · 2015-08-18T14:59:38Z

FWIW, I found the broken test, but the spawn test is still hanging on windows. Could somebody who has a running windows install test what exactly is broken there?

vtjnash · 2015-08-18T16:27:26Z

test/spawn.jl

-    @test readall(pipe(`$exename -f -e 'println(STDERR,"Hello World")'`, stderr=`cat`)) == "Hello World\n"
+    @test readall(pipeline(`$exename -f -e 'println(STDERR,"Hello World")'`, stderr=`cat`)) == "Hello World\n"
+    p = Pipe()
+    run(pipeline(`$exename -f -e 'println(STDERR,"Hello World")'`, stderr = p))


the problem i have with this API is that it looks like you could use p repeated to aggregate the stderr for many processes, but in fact you can't even call run twice on this Cmd object, since run mutates the p object when it opens it. What is the disadvantage to making Pipe refer to the entire pipe, while adding an internal type that represents just one end of the pipe (e.g. what is called Pipe in this PR)?

I think this is similar to what I proposed at some point – the idea was that a Pipe object would be similar to a cat process but without any process, just going through the kernel.

use spawn not run here to avoid forcing the kernel to buffer the output (the windows kernel is forcing the process to stay alive until there is a reader for the stderr pipe causing the hang on appveyor).

will push a fix shortly.

here's an even better example of why we should never export PipeEnd to the user:

p = PipeEnd() spawn(pipeline(`foo` & `bar`, stdout=p)) # this expression has undefined behavior, since bar may get the wrong end of the pipe

(i'm most of the way done with a cleanup effort to add a Pipe object which tracks a pair of pipe ends)

…. also rename Pipe to PipeEndpoint

tkelman added the io Involving the I/O subsystem: libuv, read, write, etc. label Jun 28, 2015

tkelman reviewed Jun 28, 2015
View reviewed changes

mbaz mentioned this pull request Jun 29, 2015

Add STDERR pipe to readandwrite #11824

Closed

Keno added this to the 0.4.0 milestone Jul 10, 2015

JeffBezanson force-pushed the kf/stderrpipe branch from 7a30092 to 491d310 Compare July 10, 2015 21:48

JeffBezanson mentioned this pull request Jul 10, 2015

possibly fix a libuv assertion failure on shutdown #12110

Closed

vtjnash reviewed Jul 13, 2015
View reviewed changes

Keno force-pushed the kf/stderrpipe branch 2 times, most recently from 9f74adf to caefce9 Compare July 14, 2015 22:30

Keno force-pushed the kf/stderrpipe branch from 6bdd44f to 2eae18b Compare July 17, 2015 19:05

Keno force-pushed the kf/stderrpipe branch from 2eae18b to d6f0076 Compare July 17, 2015 21:49

JeffBezanson force-pushed the kf/stderrpipe branch from d6f0076 to 544b9b0 Compare July 27, 2015 17:38

ViralBShah added the priority This should be addressed urgently label Aug 14, 2015

Keno added 2 commits August 16, 2015 17:59

Rename pipe -> pipeline

a201a5b

Keno force-pushed the kf/stderrpipe branch from 36fa587 to a201a5b Compare August 16, 2015 16:00

Remove a test that no longer tests what it thinks it tests

ac569bd

vtjnash reviewed Aug 18, 2015
View reviewed changes

StefanKarpinski assigned Keno Aug 19, 2015

Keno assigned vtjnash and unassigned Keno Aug 19, 2015

fix spawn test to call readall before waiting for the process to exit…

10966d7

…. also rename Pipe to PipeEndpoint

JeffBezanson mentioned this pull request Aug 21, 2015

process and pipe cleanup #12739

Merged

vtjnash closed this Aug 21, 2015

vtjnash deleted the kf/stderrpipe branch August 27, 2015 04:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it possible again to read STDERR without calling `cat` #11919

Make it possible again to read STDERR without calling `cat` #11919

Keno commented Jun 28, 2015

tkelman Jun 28, 2015

Keno Jun 28, 2015

StefanKarpinski Jun 28, 2015

JeffBezanson Jun 28, 2015

StefanKarpinski Jun 28, 2015

JeffBezanson Jul 15, 2015

Keno Jul 15, 2015

Keno Jul 15, 2015

StefanKarpinski Jul 15, 2015

Keno Jul 16, 2015

vtjnash Jul 13, 2015

vtjnash Jul 14, 2015

Keno Jul 14, 2015

tkelman commented Jul 14, 2015

Keno commented Jul 17, 2015

JeffBezanson commented Jul 28, 2015

Keno commented Jul 28, 2015

Keno commented Aug 18, 2015

vtjnash Aug 18, 2015

StefanKarpinski Aug 18, 2015

vtjnash Aug 19, 2015

vtjnash Aug 21, 2015

@@ @@ -1149,6 +1149,7 @@ export @@
                   ntoh,
                   open,
                   pipe,
+                  Pipe,

Make it possible again to read STDERR without calling cat #11919

Make it possible again to read STDERR without calling cat #11919

Conversation

Keno commented Jun 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Jul 14, 2015

Keno commented Jul 17, 2015

JeffBezanson commented Jul 28, 2015

Keno commented Jul 28, 2015

Keno commented Aug 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make it possible again to read STDERR without calling `cat` #11919

Make it possible again to read STDERR without calling `cat` #11919