Windows Process API

[jan-zajic]

Replace Fork–exec Unix technique with CreateProcess API on MS Win. As Windows processes don't use the *nix fork idiom, there is no analogue in Windows to forking. Therefore fork without exec on Win is not implemented (it's almost impossible and not so useful on Windows).

Personally tested and working.

Little bit problematic because of currently win implementation of Crystal::EventLoop, but works, so will simplify further development of windows fork. (raise "No runnables in scheduler" must be removed, see changes, then wait on Process result using Channel works, otherwise it hang).

At the same time Signal API reduced to minimal Win supported signals. Also Signal api is not so useful on Win platform.

Part of #5430.
Existing discussion to signals is here: #7339.

There is lot of necessary magic on Win platform when calling CreateProcessW and related apis..

[jkthorne]

this looks like a lot of work. Thank you for pushing forward Windows support.

[straight-shoota]

This is really awesome. Thank you very much!

It's also quite a lot of code and a huge task to review. The overall design looks good to me. But we'll certainly have to talk about the public API in detail.

We should try to split this feature into smaller bites of self-contained changes, to separate commits and preferably even separate PRs. I assume some parts can be extracted and merged on their own. This makes it easier to review and discuss each individual feature without getting lost.

Skimming over it, I could think of a few ideas:

• lib_c re-orderings
• Auxiliaries such as to_unsafew (which is not an ideal name).
• Extract POSIX implementation of Process and Signal to a portable API
• Implement Process and Signal for Win32 (maybe this can be even further broken down?)

[ysbaddaden]

Thanks for initiating this!

I think the one thing I dont like are the Signal changes. The only reason we need them with Process is to know and report when a spawned process has terminated on POSIX (SIGCHLD), which is achieved differently on Windows. The sigset is only there because of how we receive/handle signals with the EventLoop and fork/exec semantics; it's not needed on Windows.

I would leave Signal alone for the time being, not abstracting it into System (yet) and obviously not requiring it on win32. This would help to focus the pull request on Process itself for Win32. It only needs Signal on POSIX.

Last but not least, having focused commits, instead of one big commit, as proposed by @straight-shoota would be very appreciated.

[jan-zajic]

@ysbaddaden, @straight-shoota thank you for first feedback. I made these changes:

• removed completely Signal changes from this PR and exclude it on Win platform (@ysbaddaden as it's currently required in Process:Status as well, see commit c704eb4)
• removed to_unsafew for now, it will be another PR after merge this (it will unify duplicate code that convert Crystal String to windows wide character string in dir, file, process and so on)
• split remaining stuff to multiple commits

[asterite]

@jan-zajic Only to add to what others already said: this is excellent, thank you so much! With efforts like these Windows support is closer and closer. I'll try to review the commits during these days if I can, though I'll admit I'm a bit clueless regarding OS processes, fork, signals, and of course the Windows API.

But at this point I think it's better to get something working and being able to bootstrap the compiler to Windows. At that point development becomes much easier and we can worry more about the small details or performance matters.

Again, thank you! ❤️❤️❤️

[straight-shoota]

It would also be nice if you could add some comments explaining the reasoning behind some changes. For example when some code is excluded on win32 or entirely removed: Why is this necessary, and is it only a temporary solution that needs improvement later, or won't it ever work/is unnecessary, or are there alternatives?

@Sija Please don't suggest circumstantial style improvements while we still haven't discussed the overall API and implementation. This is just unnecessary noise at the moment.
@jan-zajic Maybe next time submit such a PR as a draft.

[straight-shoota]

The most important feature is the public API design for Process. There need to be some changes to remove POSIXisms and make it portable.

All documentation for public API should be directly in src/process.cr. The documentation needs to be universal as does the behaviour. It should mention platform-specific details, though.
We could consider moving the platform-specific implementation entirely into Crystal::System::Process and delegate to that from Process (there are already examples for this in the file API). Maybe it would be easier to just reopen Process and either delegate to the platform-specific methods or utilize previous_def for attaching the documentation. I think the latter is what I'd prefer. It avoids duplicate internal methods.

• Process.fork should probably be removed entirely. But that's a different story (see Process.fork has dangerous semantics #6421).
• Maybe Process.chroot as well.
• Return type for process ids should be Int32.
• I'm not sure about the proper API for sending signals/killing a process

Relevant precursor PR: #6744

[jan-zajic]

@straight-shoota, @asterite and all other - i was thinking very much about that and here (#8524) is my proposal how I think it's the best to deal with public platform dependent apis. Please let's discuss it there, before we continue our work on this Process related PR ..

[ysbaddaden]

Looking at Go, it looks like both Process.pid and .ppid can be implemented on Windows with getCurrentProcessId (already did) and GetProcessEntry for PPID which look compatible with POSIX (just integers). Other methods like .pgid (and uid, gid, euid, egid) are dummies returning -1. See https://golang.org/pkg/os.

I propose to:

• keep most methods into Process itself (instead of crystal/system/process) and follow up on Go with pid_system, (already done) ppid_system and pgid_system methods;
• keep fork buried in crystal/system/process, we're leaning on removing it anyway (with exec, exec_internal, fork_internal this becoming internal API for POSIX);
• remove most LibC type constraints (e.g. LibC::PidT) and always return an Int64 —pid_t is always an Int32 and gid_t and pid_t always an UInt32 anyway — I don't know about Windows PID, but Go merely casts them all to the target specific int, that would stop leaking LibC types to the stdlib;

Maybe:

• use Process#terminate instead of #terminate_gracefully and Process#kill instead of #terminate_immediately; they look simpler to me, yet explicit;
• rename Process#kill(Signal) to Process#signal(Signal) that we can eventually implement on Windows (if needed) with a deprecation notice.

This is mostly details to start shaping the public API. I hope we can agree and quickly merge the Windows implementation of Process!

[straight-shoota]

Sounds good. So do we agree the system implementation should provide the following interface:

self.exit_system(status : Int32) : NoReturn
self.pid_system : Int64
self.pgid_system : Int64
self.ppid_system : Int64
self.exist_system?(pid : Int64) : Bool
self.signal_system(pid : Int64, signal : Signal) : Nil
self.terminate_system(pid : Int64) : Nil
self.kill_system(pid : Int64) : Nil
system_close : Nil
create_and_exec(*args) : Int64
self.prepare_shell(command : String, args : Enumerable(String)) : {String, Enumerable(String)}
self.wait(pid : Int64)

All these methods should probably be private or protected. Those with _system suffix should directly relate to public methods without the suffix.

One issue exists for sending signals to or kill/terminate to arbitrary processes (by process id). On POSIX this works fine one process id but the windows implementations need process handle or thread id. When there are only class methods receiving pid, the windows implementation needs to lookup the handle/thread id and can't re-use the one from an instance. Alternatively, we could implement both class methods receiving pid and instance methods which can use available information. Another solution might be to re-purpose Process to also function as a handle for external processes and only have instance methods for signals etc. But that would mean most of the regular API wouldn't work on instances, so it's probably not a good idea.

[jan-zajic]

I can implement ppid, but please be aware that pid will be reused by other process if parent terminates and we don't open handle to the parent process. It's different from Linux behavior.

Regarding kill methods - i'm fine with kill/terminate if we rename existing kill method and you ok with breaking change. I try named them different to just to differentiate from that current Unix kill method which means send any signal and not send kill signal in accordance with Unix kill command.

[jan-zajic]

Btw @ysbaddaden i forget to say you that i really don't known about some GetProcessEntry windows api, it's only name of the function in go where it is implemented.. NtQueryInformationProcess must be involved. Strictly speaking, the parent process (the process which created the child process) is not actually recorded. InheritedFromUniqueProcessId just gives you the process from which attributes were inherited. This is very rarely a problem, but is controllable from the CreateProcess call, and can be something other than parent.

[straight-shoota]

We can surely defer the implementation of ppid for win32. It doesn't need to be in this PR.

[jan-zajic]

• to api proposal - system_close i change to close_system and wait to wait_system to be in line with others
• ppid - Implemented
• OLD kill - renamed to signal, but keeped only in unix/process - if i put it in win process it make dependency to Signal which is currently excluded as @ysbaddaden ask at begining. So keep it only in Unix/process and make another PR on signals later, or i can bring back signal refactoring to this PR..
• pgid i currently removed from Process api and don't want to give it back - it has same semantic as fork etc - Windows don't have any process groups. In Current Win there is similar concept of job, but it is only on demand, ie there is no job for Process unless implicitly created and jobs have string names, not any ID's ..

According to that non-portable apis, as it is last remaining design decision necessary to do:

i currently keep pgid, signal, fork and chroot as public methods in file unix/process. They are added to reopened class Process.

We can (please decide per-method):

• keep it as it is. (i.e. leave it for later decision on remove..?).
• Create these methods in file /process.cr, rename them to _system in impl and implement them as raise in Windows version.
• remove them completely. (and maybe implement them in separate shard).

[straight-shoota]

It's okay to not implement the methods in question for win32.
But they should be kept available for now. Since they're public API, they should be in src/process.cr. I'd suggest to implement them directly there behind a flag to keep the friction low. That's just meant as an interim solution until we decide later whether and how to port them for win32 or remove them. That's out of scope here, so let's keep the changes minimal.

Regarding #signal we should consider to postpone the rename, or rather move it out of this PR. It's not relevant for win32 implementation.

Could you document the internal interface for Process's system implementation in src/crystal/system/process.cr? Just copy #8518 (comment) plus your modifications and maybe a few comments. This should be similar to other files in that directory (but without a Crystal::System namespace type).

[asterite]

Splitting this PR into commits is good, but I think it's better to split this into smaller PRs. Reviewing 31 files changed and 1000 lines of code is a task I can't handle right now, maybe it's the same for others.

Also, because it's so big many discussions are all discussed in a single place and it's not easy to follow.

[jan-zajic]

Ok let's make it step by step. Closed in favor to #8536 and #8537. After we stabilize API in #8536 i will make PR's for the rest.

[RX14]

I know everyone wants to get stuck into the code as soon as possible, but to prevent this in the future, please open an issue before sending a PR.

We have that in the contribution guide, but it's worth repeating: always make an issue about API changes before sending a PR. Whether you've written the code or not.

[oprypin]

The remainder of this PR merged with master is https://github.com/oprypin/crystal/compare/winapi#files_bucket (504bcc6 as of writing this)

I am still planning to try to get that merged, but to be fair I don't quite understand it.

FYI @kubo. If you want to take that instead, let me know in advance.