Process improvements #3079

jhass · 2016-08-02T10:47:43Z

No description provided.

RX14 · 2016-08-02T11:12:52Z

src/process.cr

@@ -47,8 +47,12 @@ class Process
    nil
  end

-  def self.ppid : LibC::PidT


Why is this removed?

Ah, looks like you moved the ppid method in the file and the addition and deletion got split between two commits.

Right, made a mistake while committing, fixed.

ysbaddaden · 2016-08-02T13:00:51Z

src/process.cr

  def self.pgid : LibC::PidT
    pgid(0)
  end

+  # Returns the process group identifier of the process with the process
+  # identifier *pid*.


What about:

Returns the process group identifier of a process identified by pid.

ysbaddaden · 2016-08-02T13:12:29Z

What about moving the Process instance methods from src/process/run.cr to src/process.cr while we're at it?

ysbaddaden · 2016-08-02T13:22:35Z

Nice refactor, just a few improvements and it's 👍 to merge from me!

jhass · 2016-08-02T14:28:04Z

If anything I would collapse all of run.cr into process.cr, the instance methods make more sense in run.cr to me since it also contains the primary way of constructing a Process instance in the first place, while process.cr currently just has class level helpers. I'm on the fence of whether I'd like them combined, so should I do it here or not?

asterite · 2016-08-02T14:35:54Z

I wouldn't mind merging the two files into one

asterite · 2016-08-02T19:11:44Z

src/process.cr

+  end
+
+  private def exec_io_for(io)
+    # TODO: Compiler bug: The union doesn't properly degrade to IO::FileDescriptior|Bool


@jhass why is this method needed? If I replace this method body with just io it continues to compile and run just fine.

~~Did you try make spec?~~

Ah actually this is from before I had exec_internal, if you add type restrictions to it, it resurfaces:

diff --git a/src/process.cr b/src/process.cr index a0ca02f..29f333a 100644 --- a/src/process.cr +++ b/src/process.cr @@ -235,9 +235,9 @@ class Process argv, env, clear_env, - exec_io_for(fork_input || input), - exec_io_for(fork_output || output), - exec_io_for(fork_error || error), + fork_input || input, + fork_output || output, + fork_error || error, chdir ) rescue ex @@ -375,7 +375,7 @@ class Process end # :nodoc: - protected def self.exec_internal(command : String, argv, env, clear_env, input, output, error, chdir) + protected def self.exec_internal(command : String, argv, env, clear_env, input : Bool|IO::FileDescriptor, output : Bool|IO::FileDescriptor, error : Bool|IO::FileDescriptor, chdir) reopen_io(input, STDIN, "r") reopen_io(output, STDOUT, "w") reopen_io(error, STDERR, "w")

So I can remove the method and leave out the restrictions, working around the bug in another way, or add the restrictions so the method becomes necessary again and serves as a reminder about the bug. What do you prefer?

Thanks, I was able to reproduce it, I'll try to see what's going on. Later I'll report an issue, for now you can leave it as you prefer.

@jhass Mmm... input can be nil, the restriction on run is Stdio which includes Nil. So I think the error is correct. Or am I missing something?

If it's indeed nil, needs_pipe? returns true, so fork_input is taken, I guess the actual bug was that a not_nil! didn't help either.

Well, it brings something back, I can no longer reproduce it complaining about nil instead of MemoryIO:

diff --git a/src/process.cr b/src/process.cr index 5d9bff5..c683032 100644 --- a/src/process.cr +++ b/src/process.cr @@ -235,9 +235,9 @@ class Process argv, env, clear_env, - fork_input || input, - fork_output || output, - fork_error || error, + (fork_input || input).not_nil!, + (fork_output || output).not_nil!, + (fork_error || error).not_nil!, chdir ) rescue ex @@ -360,7 +360,7 @@ class Process end # :nodoc: - protected def self.exec_internal(command : String, argv, env, clear_env, input, output, error, chdir) + protected def self.exec_internal(command : String, argv, env, clear_env, input : Bool|IO::FileDescriptor, output : Bool|IO::FileDescriptor, error : Bool|IO::FileDescriptor, chdir) reopen_io(input, STDIN, "r") reopen_io(output, STDOUT, "w") reopen_io(error, STDERR, "w")

make spec errors with:

in ./src/process.cr:233: no overload matches 'Process.exec_internal' with types String, Array(Pointer(UInt8)), Nil, Bool, (IO::FileDescriptor | MemoryIO), IO::FileDescriptor, IO::FileDescriptor, Nil Overloads are: - Process.exec_internal(command : String, argv, env, clear_env, input : Bool | IO::FileDescriptor, output : Bool | IO::FileDescriptor, error : Bool | IO::FileDescriptor, chdir) Couldn't find overloads for these types: - Process.exec_internal(command : String, argv : Array(Pointer(UInt8)), env : Nil, clear_env : Bool, input : MemoryIO, output : IO::FileDescriptor, error : IO::FileDescriptor, chdir : Nil) Process.exec_internal( ^~~~~~~~~~~~~

Since casting to a sub-union isn't possible, this was my solution to tell Crystal what the actually possible types are at this point.

diff --git a/src/process.cr b/src/process.cr index 5d9bff5..e56defa 100644 --- a/src/process.cr +++ b/src/process.cr @@ -235,9 +235,9 @@ class Process argv, env, clear_env, - fork_input || input, - fork_output || output, - fork_error || error, + (fork_input || input).as(Bool|IO::FileDescriptor), + (fork_output || output).as(Bool|IO::FileDescriptor), + (fork_error || error).as(Bool|IO::FileDescriptor), chdir ) rescue ex @@ -360,7 +360,7 @@ class Process end # :nodoc: - protected def self.exec_internal(command : String, argv, env, clear_env, input, output, error, chdir) + protected def self.exec_internal(command : String, argv, env, clear_env, input : Bool|IO::FileDescriptor, output : Bool|IO::FileDescriptor, error : Bool|IO::FileDescriptor, chdir) reopen_io(input, STDIN, "r") reopen_io(output, STDOUT, "w") reopen_io(error, STDERR, "w")

actually crashes the compiler now that I try it.

closes crystal-lang#3077

asterite · 2016-08-03T13:29:12Z

Let's drop exec and just have run and a replace flag.

Our original Process.run design had just one run method. In Ruby there are tons of ways to do the same thing and it's pretty confusing, that's why we aimed for just one method with many options.

jhass · 2016-08-03T13:48:47Z

Process.run and Process.exec are not the same thing and they can't share the same interface, in particular passing input: nil etc. to exec makes no sense whatsoever. More importantly the sensible defaults are different. Handling all of that in run means more runtime errors and a vastly more complex implementation.

asterite · 2016-08-03T13:49:55Z

Can we then name it Process.replace? run and exec have basically the same meaning in English...

jhass · 2016-08-03T14:00:36Z

exec is well known in UNIX to replace the current process, the system call is execve (execvp etc are wrappers around it). Ruby uses exec for this operation. In Python it's os.execvpe and friends, pcntl_exec in PHP, kexec in node via the npm package of the same name, plain exec in shellscripts, basically all standard libraries that provide an interface to do it, have exec in its name.

asterite · 2016-08-03T15:32:39Z

I don't think everyone is familiar with UNIX, and having exec and run will only bring confusion. What's wrong with Process.replace? The docs say "Replaces the current process", so I see no reason to stick with exec. That's also the reason why we don't use recv in Socket. I prefer to have meaningful names over anecdotical, meaningless names.

ysbaddaden · 2016-08-03T15:40:31Z

src/process.cr

+  # are flushed and closed, all child processes are inherited by PID 1. This does
+  # not run any handlers registered with `at_exit`, use `::exit` for that.
+  #
+  # *status* is the exit status of the current process.
  def self.exit(status = 0)


Aparte:

what about renaming it to Process.exit! like in Ruby, to mark the difference and avoid a confusion?

::exit(bool) would be nice sugar too.

If those are interesting I'll propose a follow-up PR.

jhass · 2016-08-03T17:09:29Z

This is about familiarity, I brought examples of where exec is used, do you have examples of where replace or something else is used? The point is that if you're going to use this operation, you're most likely already know it from somewhere else and you'll know it as exec and look for that first.

ysbaddaden · 2016-08-03T19:57:27Z

Process.replace makes me think that I can replace any process by another process, which feels weird, whereas Process.exec feels like I'm executing something, thought one that isn't familiar with UNIX may stumble on the fact that it replaces the current process, but it's nothing that documentation can't fix. I admit I may be conservative here, but this is a POSIX only feature that only POSIX developers are aware of, so... this feels like renaming fork as duplicate: weird o.O

asterite · 2016-08-03T21:23:58Z

Let's leave it as exec for now, I guess with so many ways to spawn processes there will always be a bit of confusion (for example we have Process.run, backtick and system).

jhass · 2016-08-03T21:25:35Z

Then this is done from my side :)

drosehn · 2016-08-04T04:20:14Z

Maybe Process.replace_exec or Process.replace_me? 😃

asterite · 2016-08-04T12:19:00Z

@jhass Thank you! And thank you for also cleaning up the code and documenting it. It's super readable now!

jhass added the topic:stdlib label Aug 2, 2016

RX14 reviewed Aug 2, 2016
View reviewed changes

jhass force-pushed the process_improvements branch 2 times, most recently from c2270c3 to 23af70c Compare August 2, 2016 11:45

ysbaddaden reviewed Aug 2, 2016
View reviewed changes

jhass force-pushed the process_improvements branch from 23af70c to df78e95 Compare August 2, 2016 14:24

jhass force-pushed the process_improvements branch from df78e95 to 4783b29 Compare August 2, 2016 15:00

asterite reviewed Aug 2, 2016
View reviewed changes

jhass added 2 commits August 3, 2016 12:26

Document some Process methods

16ac8f5

Add Process.exists?, Process#exists? and Process#terminated?

fc53c5f

closes crystal-lang#3077

jhass force-pushed the process_improvements branch from 4783b29 to caecc9c Compare August 3, 2016 14:13

jhass added 2 commits August 3, 2016 16:35

Add Process.exec

1abf234

Collapse process/run.cr into process.cr

0a5c550

jhass force-pushed the process_improvements branch from caecc9c to 0a5c550 Compare August 3, 2016 14:36

ysbaddaden mentioned this pull request Aug 3, 2016

Confusing values for Process.run input, output and error #3084

Closed

ysbaddaden reviewed Aug 3, 2016
View reviewed changes

asterite merged commit 681bca3 into crystal-lang:master Aug 4, 2016

asterite added this to the 0.19.0 milestone Aug 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process improvements #3079

Process improvements #3079

jhass commented Aug 2, 2016

RX14 Aug 2, 2016

RX14 Aug 2, 2016

jhass Aug 2, 2016

ysbaddaden Aug 2, 2016 •

edited

Loading

ysbaddaden commented Aug 2, 2016

ysbaddaden commented Aug 2, 2016

jhass commented Aug 2, 2016

asterite commented Aug 2, 2016

asterite Aug 2, 2016

jhass Aug 2, 2016 •

edited

Loading

jhass Aug 2, 2016

asterite Aug 3, 2016

asterite Aug 3, 2016

jhass Aug 3, 2016

jhass Aug 3, 2016

asterite commented Aug 3, 2016

jhass commented Aug 3, 2016 •

edited

Loading

asterite commented Aug 3, 2016

jhass commented Aug 3, 2016 •

edited

Loading

asterite commented Aug 3, 2016

ysbaddaden Aug 3, 2016

jhass commented Aug 3, 2016

ysbaddaden commented Aug 3, 2016

asterite commented Aug 3, 2016

jhass commented Aug 3, 2016

drosehn commented Aug 4, 2016

asterite commented Aug 4, 2016

Process improvements #3079

Process improvements #3079

Conversation

jhass commented Aug 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysbaddaden Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

ysbaddaden commented Aug 2, 2016

ysbaddaden commented Aug 2, 2016

jhass commented Aug 2, 2016

asterite commented Aug 2, 2016

Choose a reason for hiding this comment

jhass Aug 2, 2016 • edited Loading

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

asterite commented Aug 3, 2016

jhass commented Aug 3, 2016 • edited Loading

asterite commented Aug 3, 2016

jhass commented Aug 3, 2016 • edited Loading

asterite commented Aug 3, 2016

Choose a reason for hiding this comment

jhass commented Aug 3, 2016

ysbaddaden commented Aug 3, 2016

asterite commented Aug 3, 2016

jhass commented Aug 3, 2016

drosehn commented Aug 4, 2016

asterite commented Aug 4, 2016

ysbaddaden Aug 2, 2016 •

edited

Loading

jhass Aug 2, 2016 •

edited

Loading

jhass commented Aug 3, 2016 •

edited

Loading

jhass commented Aug 3, 2016 •

edited

Loading