process and pipe cleanup #12739

vtjnash · 2015-08-21T21:04:11Z

this is a grab-bag of various fixes to the Process and Pipe handling. I'm sticking all of these together to try to minimize the incremental API churn and breakage:

this includes and thus replaces Make it possible again to read STDERR without calling cat #11919 (which also fixes Add STDERR pipe to readandwrite #11824)
fixes use of open() results or filehandles #9659 by changing open(Cmd) to only return a Process, defining IO operations on Process, and defining close(Process) to mean close(Process.in) && close(Process.out). this is a breaking change, since i know of no way to sanely provide a deprecation for this (I would prefer not to define iteration over a Process object as returning the old tuple of (io, process)) [this item is moving to a new PR]
makes Cmd objects immutable and other general API cleanup utilizing various new features in Julia like keyword arguments, immutable types
this deletes all usages of Intrinsics.Box(Ptr{Void},Intrinsics.jl_alloca(size)) from Base (preferring the possibility of a small memory leak in exceptional cases instead of segfaults), in preparation for deprecating this intrinsic function
does stdio setup in generic functional style rather than hard coded case statements and macros
adds a few more methods to DevNull (ref Enhancement: would it be possible to add lock() (and possibly other methods) to DevNullStream? #12050), although its still short of the list implemented for Pipe
also fixes InexactError on failed spawn #12176 (inexact error on spawn on windows)

JeffBezanson · 2015-08-21T21:17:19Z

base/process.jl

+iswritable(::DevNullStream) = true
+isopen(::DevNullStream) = true
+read{T<:DevNullStream}(::T, args...) = throw(EOFErorr())
+write{T<:DevNullStream}(::T, args...) = 0


This might be ok for now but I'm not sure it's right. Semantically, I believe all bytes are successfully written to DevNull.

i think you're right, i just don't know if there is a good way to generically compute the number of bytes that got "written"

That's what sizeof is for.

JeffBezanson · 2015-08-21T21:27:42Z

Awesome!! Looks really good.

Very happy to see jl_alloca go away. Do any packages use it? Would be best to remove it entirely.

It looks like the fix for #12176 can be split out? Maybe the DevNull changes too?

JeffBezanson · 2015-08-21T21:28:30Z

Also, I'm pretty sure you meant this replaces #11919, not #12113?

vtjnash · 2015-08-21T21:32:57Z

Also, I'm pretty sure you meant this replaces #11919, not # 12113?

fixed. i have no idea how that got into my paste buffer

It looks like the fix for #12176 can be split out? Maybe the DevNull changes too?

yes they could. i put them all together here mostly to see if there would be any overlapping API decisions. i generally try to structure many of my commits so that they are relatively independent and could be reordered or dropped as needed.

Very happy to see jl_alloca go away. Do any packages use it? Would be best to remove it entirely.

I'm not sure, so the deprecation should happen in a separate PR.

JeffBezanson · 2015-08-21T21:34:39Z

Looks like a small memory leak can only happen if link_pipe fails? In that case, would it make sense to put link_pipe inside the try-finally block in spawn?

vtjnash · 2015-08-21T21:48:03Z

to do it right, you would need to figure out how far into setting up stdio it got and then unwind those steps (starting at https://github.com/JuliaLang/julia/pull/12739/files#diff-ab46297895850308343e9cbd5ebeb6cdR392). it would be a good improvement (the old code didn't do it either)

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.

…. also rename Pipe to PipeEndpoint

… appropriate stdin/stdout stream

…e a hidden argument rather than an awkward first argument

PipeEndpoint has behavioral flaws when used on its own since it is incapable of tracking the full pipe and the behavior of both ends

overdue so long

…2):typemax(UInt32) that may be returned by spawn on windows (fixes #12176)

sbromberger · 2015-08-23T16:01:15Z

Thank you for including #12050.

IainNZ · 2015-08-23T16:15:38Z

Very happy to see jl_alloca go away. Do any packages use it? Would be best to remove it entirely.

https://github.com/search?l=julia&q=jl_alloca&type=Code&utf8=%E2%9C%93

Looks like no, except for various forks of Julia

ViralBShah · 2015-08-25T13:08:53Z

Is this good to merge now? Looks like the last major thing in the way for 0.4 RCs.

vtjnash · 2015-08-25T15:52:51Z

yes. however, since this is breaking for the definition of open(cmd), i was thinking of merging this as close to the branch point as possible, so that writing the deprecation in packages is a simpler test >v"0.4.0-dev"

StefanKarpinski · 2015-08-25T15:54:17Z

That is literally the worst reason to delay merging something I've ever heard.

ivarne · 2015-08-25T16:00:40Z

That is literally the worst reason to delay merging something I've ever heard.

+1

vtjnash · 2015-08-25T16:12:49Z

That's my deadline for hitting the merge button. I won't hinder anyone else from merging it sooner.

ViralBShah · 2015-08-25T18:05:46Z

Please merge. We are pretty close to the branch point.

StefanKarpinski · 2015-08-25T18:18:16Z

@JeffBezanson, please merge if you're happy with this.

JeffBezanson · 2015-08-25T20:10:57Z

My main concern is that the change to open has not really been discussed at all. All we wanted was to make it easier to read stderr, and I don't think changing open is necessary to do that. (There are also bug fixes applicable to master bundled in here, which annoys me extremely, but that's a much smaller issue right now.)

vtjnash · 2015-08-25T23:12:28Z

agreed, i've taken out the change to open in a separate revert commit so that this PR is no longer breaking and can be be merged once CI passes. Later, I'll open a second PR to revert that last commit, with more focused discussion. Including it here was initially helpful since it helped me test out these changes and account for the overlapping functionality, but the actual change doesn't need to happen in this PR merge.

for separation into a independent merge commit

JeffBezanson · 2015-08-26T02:32:59Z

That's great; then we can merge this ASAP.

process and pipe cleanup

vtjnash · 2015-08-26T02:45:26Z

That's great; then we can merge this ASAP.

yep, that was exactly the point. and as promised, the new PR is #12807

JeffBezanson · 2015-08-26T02:59:25Z

Note: needs NEWS update for the rename of pipe, and to mention Pipe().

These probably should have been cleaned up along side that in #12739. Certainly the documentation was out of date, since it referred to `Pipe`, but meant what is now called `PipeEndpoint`. Clean all of this up, by allowing an unitinalized `Pipe` to be passed to these functions, and also by replacing the tuple of `PipeEndpoint`s, by `Pipe`, since the purpose of `Pipe` is precisely to hold two endpoints.

vtjnash added the breaking This change will break code label Aug 21, 2015

JeffBezanson reviewed Aug 21, 2015
View reviewed changes

JeffBezanson added this to the 0.4.0 milestone Aug 21, 2015

vtjnash force-pushed the jn/processpipe_cleanup branch from 8d17387 to 7825dc4 Compare August 21, 2015 22:05

Keno and others added 10 commits August 21, 2015 21:07

Rename pipe -> pipeline

0c17088

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

a8a2e6d

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

8206f9f

…. also rename Pipe to PipeEndpoint

create a new Pipe type that represents an entire pipeline

6b223a3

add a few definitions to DevNullStream

6030f7e

redesign Process to be a subtype of IO and dispatch read/write to the…

80be437

… appropriate stdin/stdout stream

fix permission bits for stdio file open on *nix

18d646f

add enough methods to AbstractPipe to get the tests to pass

2ff0c54

now that we have named arguments, ProcessChainOrNot can finally becom…

fd40e79

…e a hidden argument rather than an awkward first argument

vtjnash force-pushed the jn/processpipe_cleanup branch from 7825dc4 to 2f24987 Compare August 22, 2015 01:08

vtjnash added 4 commits August 21, 2015 23:22

fix tests broken by open(cmd)=>Process change

9f43cef

always use Pipe, not PipeEndpoint, in spawn

dc03fbb

PipeEndpoint has behavioral flaws when used on its own since it is incapable of tracking the full pipe and the behavior of both ends

make Cmd objects immutable

1481981

overdue so long

Process.exitcode needs to be an Int64 to cover the range typemin(Int3…

c565cd0

…2):typemax(UInt32) that may be returned by spawn on windows (fixes #12176)

vtjnash force-pushed the jn/processpipe_cleanup branch from 2f24987 to c565cd0 Compare August 22, 2015 03:35

vtjnash removed the breaking This change will break code label Aug 25, 2015

Revert open(cmd) => Process change

8ffdfc2

for separation into a independent merge commit

vtjnash force-pushed the jn/processpipe_cleanup branch from 3c295ed to 8ffdfc2 Compare August 25, 2015 23:42

JeffBezanson added a commit that referenced this pull request Aug 26, 2015

Merge pull request #12739 from JuliaLang/jn/processpipe_cleanup

aa8cd2e

process and pipe cleanup

JeffBezanson merged commit aa8cd2e into master Aug 26, 2015

vtjnash deleted the jn/processpipe_cleanup branch August 26, 2015 02:42

vtjnash added a commit that referenced this pull request Aug 26, 2015

add NEWs entry for #12739

d536568

ViralBShah mentioned this pull request Aug 27, 2015

New warnings with 0.4 JuliaWeb/HttpServer.jl#60

Closed

tkelman mentioned this pull request Feb 28, 2016

use of open() results or filehandles #9659

Closed

Keno mentioned this pull request Aug 26, 2016

Clean up redirect_* APIs after Pipe -> PipeEndpoint rename #18253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

process and pipe cleanup #12739

process and pipe cleanup #12739

vtjnash commented Aug 21, 2015

JeffBezanson Aug 21, 2015

vtjnash Aug 21, 2015

Keno Aug 22, 2015

JeffBezanson commented Aug 21, 2015

JeffBezanson commented Aug 21, 2015

vtjnash commented Aug 21, 2015

JeffBezanson commented Aug 21, 2015

vtjnash commented Aug 21, 2015

sbromberger commented Aug 23, 2015

IainNZ commented Aug 23, 2015

ViralBShah commented Aug 25, 2015

vtjnash commented Aug 25, 2015

StefanKarpinski commented Aug 25, 2015

ivarne commented Aug 25, 2015

vtjnash commented Aug 25, 2015

ViralBShah commented Aug 25, 2015

StefanKarpinski commented Aug 25, 2015

JeffBezanson commented Aug 25, 2015

vtjnash commented Aug 25, 2015

JeffBezanson commented Aug 26, 2015

vtjnash commented Aug 26, 2015

JeffBezanson commented Aug 26, 2015

process and pipe cleanup #12739

process and pipe cleanup #12739

Conversation

vtjnash commented Aug 21, 2015

JeffBezanson Aug 21, 2015

Choose a reason for hiding this comment

vtjnash Aug 21, 2015

Choose a reason for hiding this comment

Keno Aug 22, 2015

Choose a reason for hiding this comment

JeffBezanson commented Aug 21, 2015

JeffBezanson commented Aug 21, 2015

vtjnash commented Aug 21, 2015

JeffBezanson commented Aug 21, 2015

vtjnash commented Aug 21, 2015

sbromberger commented Aug 23, 2015

IainNZ commented Aug 23, 2015

ViralBShah commented Aug 25, 2015

vtjnash commented Aug 25, 2015

StefanKarpinski commented Aug 25, 2015

ivarne commented Aug 25, 2015

vtjnash commented Aug 25, 2015

ViralBShah commented Aug 25, 2015

StefanKarpinski commented Aug 25, 2015

JeffBezanson commented Aug 25, 2015

vtjnash commented Aug 25, 2015

JeffBezanson commented Aug 26, 2015

vtjnash commented Aug 26, 2015

JeffBezanson commented Aug 26, 2015