Extract POSIX implementation of Process and Signal to a portable API #8536

jan-zajic · 2019-11-29T19:13:56Z

Prepare Process API for Windows implementation. After discussion in #8518 i extract the system implementation which provide
the following private interface (all methods are protected):

# for current running program
self.exit_system(status : Int32) : NoReturn
self.pid_system : Int64
self.ppid_system : Int64
self.fork_system(&) : Process
self.fork_system : Process?
self.chroot_system(path : String) : Nil
self.times_system : Tms

# for Process instance
terminate_system : Nil
kill_system : Nil
close_system: Nil
exist_system? : Bool
signal_system(signal : Signal) : Ni
wait_system : Nil
pgid_system : Int64
create_and_exec(command, *args, ...) : Int64

# for given pid
self.exists_system?(pid : Int64)
self.pgid_system(pid : Int64) : Int64

# util
self.prepare_shell_system(command : String, args : Enumerable(String)) : {String, Enumerable(String)}

Public API design for Process
There need to be some changes to remove POSIXisms and make it portable.

~~kill method is renamed to signal~~ move to Rename Process kill methods to signal #8642
new methods terminate (graceful), interrupt (immediately)

Precursor PR: #8518

ysbaddaden

Looks great 👍

I made some stylistic suggestions to avoid self. and Process:: when unnecessary.

I also propose to have exists_system? and pgid_system be class methods to avoid allocating a Process object just to throw it away immediately in Process.exists?(pid) and Process.pgid methods.

I believe we can already wrap the fork, chroot and pgid methods to not exist on Win32.

src/crystal/system/unix/process.cr

src/process.cr

src/crystal/system/unix/process.cr

jan-zajic · 2019-11-30T20:14:05Z

@ysbaddaden it's done, please look at it.

src/crystal/system/unix/process.cr

src/process.cr

src/crystal/system/unix/process.cr

ysbaddaden

Just one more tiny change (and Sija's stylistics suggestions are good too), but otherwise 👍 for me!

src/crystal/system/unix/process.cr

ysbaddaden · 2019-12-01T09:44:28Z

Oh, and thank you for spending so much time on making this super clean!

src/crystal/system/unix/process.cr

RX14

We can't have methods which exist on only some platforms. I've done some work on this before, so I'll dig out my old code and review it while looking at how I did this task before.

ysbaddaden · 2019-12-05T14:18:57Z

@RX14 Let's avoid holding things up and keep them hanging.

IMO: the abstraction here is good. It doesn't remove any functionality and will be enough to implement Process on Windows. We can make it perfect afterwards, deciding in their own dedicated issues, whether we keep fork, chroot or include (e)(uid|gid) or not.

src/crystal/system/unix/process.cr

src/process.cr

ysbaddaden

👍

rdp · 2019-12-17T06:37:00Z

What does exists_system mean? The system part of the method name is a bit confusing to myself... :)

RX14

Please try to keep Process's API the same, removing Process.kill must be another PR.
There's no need to attempt to abstract Process.fork, it will be removed anyway. Just abstract fork_internal enough that it can be used to implement spawn in unix.

Also see my previous attempt at this 2 years ago for reference: RX14@ed14ad5#diff-4af37cd7133b9dffdefaf5fd05463245. Many of the refactors in this commit have made their way to master after this, just not the actual move to Crystal::System.

src/process.cr

ysbaddaden · 2019-12-21T23:35:24Z

@RX14 Process.kill wasn't removed but renamed to Process.signal which is a better name.

RX14 · 2019-12-22T00:45:41Z

@ysbaddaden I agree - but it still should be in seperate PRs.

jan-zajic · 2020-01-02T15:46:19Z

Ok @RX14, @straight-shoota i move kill method renaming to new PR #8642. But because of this and our intention with @ysbaddaden to have portable API to stopping processes i rename proposed kill method that immediately terminate process to interrupt. Otherwise it would be super unclear until we remove original kill methods. The more so that someone already suggested to make it only deprecated in #8642 (which I expected).

RX14 · 2020-01-02T17:27:21Z

@jan-zajic I'd rather just have no kill or interrupt or terminate, just signal(:kill) and signal(:interrupt). On windows, signal would only be able to send sigkill, an attempt to send sigint would raise. This is similar to how go approaches it.

However, please no user-visible API changes in this PR. It'd make me far far happier to review this diff if all the methods and their signatures stayed in the same place and only their implementations changed. Or, at least if you do clean things up and rearrange, seaperate that out into into a different commit.

jan-zajic · 2020-01-03T15:32:20Z

@RX14 i rearranged it as you wish. But i insist on to have some methods to stop Process (no matter how they are named, it can be discussed). We try to create portable API (at least @ysbaddaden and i), that can be implemented on every platform. If you can create Process, you must be able to stop it. Do not confuse it with that signal stuff. It's only implementation detail on unix. Stopping or killing most of the processes on Windows, for example, have nothing to do with signal API. When you mention go for example, it also has Kill() method on Process that causes the Process to exit immediately in platform specific way. But Go lang is not a good example of pretty apis at all, please let's not be influenced by previous compromises of other teams.

jan-zajic · 2020-01-03T15:40:59Z

@bcardiff builds now starts to fail on:

HTTP::Cookie::Parser expired? not expired
Failure/Error: parse_set_cookie("bla=1; expires=Thu, 01 Jan 2020 00:00:00 -0000").expired?.should eq false
Expected: false
got: true

P.S. happy New Year to everyone involved in Crystal Language!

RX14 · 2020-01-03T15:49:52Z

Stopping or killing most of the processes on Windows, for example, have nothing to do with signal API

That is true, but that does not mean that the API for killing a process on windows cannot be exposed as signal(:kill), which is then translated to TerminateProcess. This is how go handles it, and is a subjective API design issue. I'm fine with either way.

Sija · 2020-01-03T16:07:51Z

@jan-zajic You need to rebase on master in order to get green CI builds.

RX14 · 2020-01-03T15:56:06Z

src/process.cr

@@ -186,14 +111,14 @@ class Process
  #
  # Returns the block's value.
  def self.run(command : String, args = nil, env : Env = nil, clear_env : Bool = false, shell : Bool = false,
-               input : Stdio = Redirect::Pipe, output : Stdio = Redirect::Pipe, error : Stdio = Redirect::Pipe, chdir : String? = nil)
+               input : Stdio = Redirect::Pipe, output : Stdio = Redirect::Pipe, error : Stdio = Redirect::Pipe, chdir : String? = nil, &block : Process ->)


I think &block will eventually be changed to always capture, so & : Process -> is the new syntax.

This big one line (and the above one) can be split to one argument per line.

src/process.cr

RX14 · 2020-01-03T16:07:44Z

src/process.cr

@@ -229,7 +154,7 @@ class Process
    end
  end

-  getter pid : Int32
+  getter pid : Int64 = 0


Is this default zero neccesary?

RX14 · 2020-01-03T16:16:09Z

src/process.cr

    pids.each do |pid|
-      ret = LibC.kill(pid, signal.value)
-      raise Errno.new("kill") if ret < 0
+      Process.new(pid).kill(signal)


This isn't good on unix since you need to allocate a Process and call waitpid just to send a signal. I think there needs to be both .signal_system(signal, pid) and #signal_system(signal).

signal_system is internal API, having a class method would be enough. It should be used for the (system specific) implementations of both class and instance methods kill, terminate and interrupt, respectively.
Adding an instance method #signal_system(signal) wouldn't be super useful when you can just call Process.signal_system(signal, @pid) instead. It would only be internal API anyway.

@straight-shoota that's fine on unix, but on windows you call TerminateProcess on the handle.

Converting from the process handle to the PID just to convert to the process handle is inefficient, and that optimization is why there is both a class and an instance method: the Process already has the handle open.

Oh yes, you're right.

RX14 · 2020-01-03T16:18:03Z

src/crystal/system/unix/process.cr

+    raise Errno.new("kill") if ret < 0
+  end
+
+  protected def create_and_exec(command : String, args : (Array | Tuple)?, env : Env?, clear_env : Bool, fork_input : IO::FileDescriptor, fork_output : IO::FileDescriptor, fork_error : IO::FileDescriptor, chdir : String?)


Perhaps call this just spawn?

I do t like it much, but I still prefer the explicit name than a confusing name (spawn fiber). Its internal API anyway.

RX14 · 2020-01-03T16:20:48Z

src/process.cr

-  def self.pgid : Int64
-    pgid(0)
-  end
+  {% if flag?(:unix) || flag?(:docs) %}


All these methods must exist on windows, but raise at runtime. i.e. delete this entire commit and raise inside pgid_system in src/crystal/system/win32/process.cr.

But if one use this methods on Windows, this error will be raised only at runtime, despite being catchable a compile-time.

At least, maybe having a warning on this ones for Windows.

@j8r This PR is mostly an internal refactor to extract the POSIX implementation. Not a place to discuss API changes.
I think we agreed to eventually remove fork and chroot, so they don't need to exist for win32. I'm not sure about the current discussion on pgid. @ysbaddaden and @jan-zajic voted to treat it like fork and chroot because it's a very POSIX-specific thing.

fork is going, and will not be in 1.0 in any form. That's why it's special.

Every other API has to be identical at compile time on every platform. This was agreed in an issue, and is the status quo across the whole rest of the stdlib: see NotImplementedError.

Yes, the question is: will process group id be in 1.0 or not? If I understood correctly, this is supposed to go as well.

@straight-shoota where's the discussion on that?

To my knowledge there has not been a dedicated discussion on that specific feature. But it has been mentioned here and in #8515. There were no objections voiced, so it seems as if everyone is on board with this.

RX14 · 2020-01-03T16:22:52Z

src/process.cr

@@ -273,6 +273,16 @@ class Process
    @waitpid.closed? || !exists_system?
  end

+  # Terminate process gracefully


Does this exist on windows?

How? I can't see a way to terminate an arbitrary process gracefully. Go doesn't support it.

Most of time you deal with GUI process, because Windows is about, surprise, windows ;). So you must find main window of target process and send WM_CLOSE message to it. If window not found, you can send WM_QUIT message to main thread. For console process, you should send crtl+c event to it using GenerateConsoleCtrlEvent.

Go doesn't seem to want to deal with that complexity for graceful process termination. I guess we'll see how much code there is :)

j8r · 2020-01-04T16:26:58Z

src/crystal/system/unix/process.cr

+        io << ' '
+        arg.inspect(io)
+      end
+      io << ")"


Suggested change

io << ")"

io << ')'

j8r · 2020-01-04T16:31:20Z

src/crystal/system/unix/process.cr

+      usage.ru_utime.tv_sec.to_f64 + usage.ru_utime.tv_usec.to_f64 / 1e6,
+      usage.ru_stime.tv_sec.to_f64 + usage.ru_stime.tv_usec.to_f64 / 1e6,
+      child.ru_utime.tv_sec.to_f64 + child.ru_utime.tv_usec.to_f64 / 1e6,
+      child.ru_stime.tv_sec.to_f64 + child.ru_stime.tv_usec.to_f64 / 1e6,


The #to_f64 are not needed anymore since #8120

Good catch. But I'd prefer to put such changes in a separate PR.

jan-zajic mentioned this pull request Nov 29, 2019

Windows Process API #8518

Closed

ysbaddaden reviewed Nov 30, 2019

View reviewed changes

jan-zajic force-pushed the feature/process_system branch from a986ebd to 2006c6a Compare November 30, 2019 20:09

jan-zajic requested a review from ysbaddaden November 30, 2019 20:14

Sija reviewed Dec 1, 2019

View reviewed changes

ysbaddaden approved these changes Dec 1, 2019

View reviewed changes

src/crystal/system/unix/process.cr Show resolved Hide resolved

src/crystal/system/unix/process.cr Outdated Show resolved Hide resolved

src/crystal/system/unix/process.cr Outdated Show resolved Hide resolved

Sija reviewed Dec 1, 2019

View reviewed changes

src/crystal/system/unix/process.cr Outdated Show resolved Hide resolved

RX14 reviewed Dec 5, 2019

View reviewed changes

jan-zajic added a commit to jan-zajic/crystal that referenced this pull request Dec 6, 2019

crystal-lang#8536 style improvements

246670d

Sija reviewed Dec 6, 2019

View reviewed changes

src/crystal/system/unix/process.cr Outdated Show resolved Hide resolved

src/crystal/system/unix/process.cr Outdated Show resolved Hide resolved

ysbaddaden reviewed Dec 6, 2019

View reviewed changes

src/process.cr Outdated Show resolved Hide resolved

jan-zajic added a commit to jan-zajic/crystal that referenced this pull request Dec 6, 2019

crystal-lang#8536 style improvements

4c61ae8

jan-zajic force-pushed the feature/process_system branch from 246670d to 4c61ae8 Compare December 6, 2019 21:15

jan-zajic added a commit to jan-zajic/crystal that referenced this pull request Dec 6, 2019

crystal-lang#8536 revert unwanted new process api open method

45d6a0d

ysbaddaden approved these changes Dec 7, 2019

View reviewed changes

RX14 requested changes Dec 21, 2019

View reviewed changes

src/process.cr Outdated Show resolved Hide resolved

jan-zajic mentioned this pull request Jan 2, 2020

Rename Process kill methods to signal #8642

Closed

jan-zajic force-pushed the feature/process_system branch from 45d6a0d to 41f6e41 Compare January 2, 2020 15:39

jan-zajic force-pushed the feature/process_system branch 2 times, most recently from d2f6b8b to d34295f Compare January 3, 2020 15:20

jan-zajic added 3 commits January 3, 2020 17:12

Extract POSIX implementation of Process and Signal to a portable API

66cdbaa

wrap the fork, chroot and pgid methods to not exist on Win32

43a2502

portable API to stop Process

224643f

jan-zajic force-pushed the feature/process_system branch from d34295f to 224643f Compare January 3, 2020 16:12

RX14 requested changes Jan 3, 2020

View reviewed changes

j8r reviewed Jan 4, 2020

View reviewed changes

oprypin mentioned this pull request Apr 10, 2020

Add Crystal::System::Process to split out system-specific implementations #9035

Merged

RX14 closed this in #9035 Apr 13, 2020

Extract POSIX implementation of Process and Signal to a portable API #8536

Extract POSIX implementation of Process and Signal to a portable API #8536

Conversation

jan-zajic commented Nov 29, 2019 • edited

ysbaddaden left a comment

Choose a reason for hiding this comment

jan-zajic commented Nov 30, 2019

ysbaddaden left a comment

Choose a reason for hiding this comment

ysbaddaden commented Dec 1, 2019

RX14 left a comment

Choose a reason for hiding this comment

ysbaddaden commented Dec 5, 2019

ysbaddaden left a comment

Choose a reason for hiding this comment

rdp commented Dec 17, 2019

RX14 left a comment • edited

Choose a reason for hiding this comment

ysbaddaden commented Dec 21, 2019 • edited

RX14 commented Dec 22, 2019 • edited

jan-zajic commented Jan 2, 2020 • edited

RX14 commented Jan 2, 2020 • edited

jan-zajic commented Jan 3, 2020

jan-zajic commented Jan 3, 2020

RX14 commented Jan 3, 2020

Sija commented Jan 3, 2020

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

ysbaddaden Jan 4, 2020 • edited

Choose a reason for hiding this comment

RX14 Jan 3, 2020 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-zajic commented Nov 29, 2019 •

edited

RX14 left a comment •

edited

ysbaddaden commented Dec 21, 2019 •

edited

RX14 commented Dec 22, 2019 •

edited

jan-zajic commented Jan 2, 2020 •

edited

RX14 commented Jan 2, 2020 •

edited

ysbaddaden Jan 4, 2020 •

edited

RX14 Jan 3, 2020 •

edited