Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

process and pipe cleanup #12739

Merged
merged 15 commits into from Aug 26, 2015

Conversation

8 participants
@vtjnash
Copy link
Member

commented Aug 21, 2015

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 #11919 (which also fixes #11824)
  • fixes #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 #12050), although its still short of the list implemented for Pipe
  • also fixes #12176 (inexact error on spawn on windows)

@vtjnash vtjnash added the breaking label Aug 21, 2015

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

This comment has been minimized.

Copy link
@JeffBezanson

JeffBezanson Aug 21, 2015

Member

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

This comment has been minimized.

Copy link
@vtjnash

vtjnash Aug 21, 2015

Author Member

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"

This comment has been minimized.

Copy link
@Keno

Keno Aug 22, 2015

Member

That's what sizeof is for.

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Aug 21, 2015

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

This comment has been minimized.

Copy link
Member

commented Aug 21, 2015

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

@vtjnash

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2015

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

This comment has been minimized.

Copy link
Member

commented Aug 21, 2015

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?

@JeffBezanson JeffBezanson added this to the 0.4.0 milestone Aug 21, 2015

@vtjnash

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2015

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)

@vtjnash vtjnash force-pushed the jn/processpipe_cleanup branch from 8d17387 to 7825dc4 Aug 21, 2015

Keno and others added some commits Jun 28, 2015

@vtjnash vtjnash force-pushed the jn/processpipe_cleanup branch from 7825dc4 to 2f24987 Aug 22, 2015

vtjnash added some commits Aug 21, 2015

always use Pipe, not PipeEndpoint, in spawn
PipeEndpoint has behavioral flaws when used on its own since it is incapable of tracking the full pipe and the behavior of both ends
Process.exitcode needs to be an Int64 to cover the range typemin(Int3…
…2):typemax(UInt32) that may be returned by spawn on windows (fixes #12176)

@vtjnash vtjnash force-pushed the jn/processpipe_cleanup branch from 2f24987 to c565cd0 Aug 22, 2015

@sbromberger

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2015

Thank you for including #12050.

@IainNZ

This comment has been minimized.

Copy link
Member

commented Aug 23, 2015

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

This comment has been minimized.

Copy link
Member

commented Aug 25, 2015

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

@vtjnash

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2015

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

This comment has been minimized.

Copy link
Member

commented Aug 25, 2015

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

@ivarne

This comment has been minimized.

Copy link
Member

commented Aug 25, 2015

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

+1

@vtjnash

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2015

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

@ViralBShah

This comment has been minimized.

Copy link
Member

commented Aug 25, 2015

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

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Aug 25, 2015

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

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Aug 25, 2015

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 vtjnash removed the breaking label Aug 25, 2015

@vtjnash

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2015

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.

Revert open(cmd) => Process change
for separation into a independent merge commit

@vtjnash vtjnash force-pushed the jn/processpipe_cleanup branch from 3c295ed to 8ffdfc2 Aug 25, 2015

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Aug 26, 2015

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

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

@JeffBezanson JeffBezanson merged commit aa8cd2e into master Aug 26, 2015

3 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@vtjnash vtjnash deleted the jn/processpipe_cleanup branch Aug 26, 2015

@vtjnash

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2015

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

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

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Aug 26, 2015

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

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

@ViralBShah ViralBShah referenced this pull request Aug 27, 2015

Closed

New warnings with 0.4 #60

Keno added a commit that referenced this pull request Aug 26, 2016

Clean up redirect_* APIs after Pipe -> PipeEndpoint rename
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.