process_release/1 and process_wait/2 api cleanup

[danilp-id]

From swi-prolog version of process module:

process_release(+PID)
Release process handle. In this implementation this is the same as process_wait(PID, _).

in swi-prolog:

?- process_create(path(true), [], [process(P)]), process_wait(P, R), process_id(P, ID).
P = ID, ID = 337330,
R = exit(0).

?- process_create(path(true), [], [process(P)]), process_release(P), process_id(P, ID).
P = ID, ID = 337333.

in scryer:

?- process_create("true", [], [process(P)]), process_wait(P, R), process_id(P, ID).
   P = '$process'(337810), R = exit(0), ID = 337810.
?- process_create("true", [], [process(P)]), process_release(P), process_id(P, ID).
   error(existence_error(process,'$dropped_value'),[predicate-process_id/2|process_id/2]).

I think process_wait/2 should do all the cleanup. Also, is there any reason to wrap a process in a custom struct? Why not just pass the process id around like swi does it?

[Skgland]

?- process_create("true", [], [process(P)]), process_release(P), process_id(P, ID).
   error(existence_error(process,'$dropped_value'),[predicate-process_id/2|process_id/2]).

This is expected, process != pid and you already released the process reference so we don't have anything to lookup the pid anymore.
In swi this works because process == pid and process_id/2 is just process_id(ID, ID).

---

I think process_wait/2 should do all the cleanup.

That would mean you couldn't release the process reference without waiting for the process to exit.
process_release/1 is there to release the process object when you don't want to wait for the process to exit, you wouldn't normally use it.

From the sictus prolog documentation

process_release(+Process)
Release a process reference Process that has previously been obtained from
process_create/3. This ensures that Prolog and the operating system can
reclaim any resources associated with the process reference.
Usually you would not call this. Either do not request the process reference
when calling process_create/3 or let process_wait/[2,3] reclaim the process reference when the process terminates.

---

Also, is there any reason to wrap a process in a custom struct? Why not just pass the process id around like swi does it?

There are multiple reasons for this:

• compatibility with swi and sictus by mirroring the stricter API
  • programs written against the stricter API should just work against the looser one
• rust does not have an API for waiting/killing a process by pid
• prevent confusion between integers and processes, catche errors due in case someone accidentally swaps variables
• prevent problems due to pid reuse (use-after-exit, use-after-free)

From swi-prolog docs

Incompatibilities and current limitations

Where SICStus distinguishes between an internal process id and the OS process id, this implementation does not make this distinction. This implies that is_process/1 is incomplete and unreliable.

From the sictus prolog documentation

Both SICStus and the operating system maintain a state for each such process reference and they must
therefore be released, either explicitly with process_release/1 or implicitly by process_
wait/[2,3].

[triska]

Thank you both for looking into this!

In case of doubt, I strongly recommend aiming for SICStus compatibility!

[Skgland]

Oh, I noticed sictus process_wait/3 has a release(Bool) option, having the option to not release the process reference on wait sounds useful.

[danilp-id]

I am curious as to how rust can guarantee that its process identifier does not collide with a different system process.

What if child process dies before the handle is released and then a new process is spawned with a colliding pid? (though this probably won't usually happen as kernel does try not to reuse pids too quickly). This might happen if someone maliciously would create a ton of processes though.

[Skgland]

Oh, wait

?- process_create("true", [], [process(P)]), process_wait(P, R), process_id(P, ID).
  P = '$process'(337810), R = exit(0), ID = 337810.

It looks like process_wait/2 currently acts as if option release(false) was set instead of defaulting to release(true).

[triska]

how rust can guarantee that its process identifier does not collide with a different system process.

The Rust level can guarantee a unique ID by never reusing Rust-internal IDs. For instance, every process that is created could get a new Rust-internal integer-ID, and already used IDs would only be reused if they have been explicitly freed.

[Skgland]

I am curious as to how rust can guarantee that its process identifier does not collide with a different system process.

What if child process dies before the handle is released and then a new process is spawned with a colliding pid? (though this probably won't usually happen as kernel does try not to reuse pids too quickly). This might happen if someone maliciously would create a ton of processes though.

The short answer is: it doesn't

The long answer is

On Linux if you drill down¹ you will see that a process is currently identified by a pid.
There is work in progress on allowing to additionally capture a PidFd when requested.
The difference between a pid and a PidFd is similar to that between a file path and a file descriptor (fd).
The resource under a pid/file path can change without notice while the resource for a pid_fd/fd stays around/the same while it is opened.

On windows it already uses a Handle rather than a pid².

Likely due to the reason layed out in the sictus documentation

Under Windows, it is not possible to obtain the exit status using a process identifier if the process
has already exited.

So using a pid wouldn't work on windows anyways.

Footnotes

1. https://doc.rust-lang.org/src/std/sys/process/unix/unix.rs.html#951 ↩

2. https://github.com/rust-lang/rust/blob/main/library/std/src/sys/process/windows.rs#L685 ↩

[danilp-id]

PidFd is nice! Should definitely add an option to turn it on when it becomes stable (or maybe use it by default to make api the same to windows, and add an option to opt out if needed for performance). Maybe we can already enable it under #[feature(linux_pidfd)] rust feature flag for nightly builds?

[Skgland]

Rust Tracking Issue for added context rust-lang/rust#82971

PidFd is nice! Should definitely add an option to turn it on when it becomes stable (or maybe use it by default to make api the same to windows, and add an option to opt out if needed for performance).

Yeah, I was planing to make it the default once it is stable.
A bit surprised they are making it opt-in rather than opt-out.
Probably some backwards compatibility concern regarding the amount of open file discriptors which can be rather limited.

Currently we store a Child which besides the Process also has space for Stdin, Stdout and Stderr, But for us those will always be empty as we either don't create them or remove them directly after spawning the child.
On linux, if the user dosen't opt-out, we would then be able to store just a PidFd and pid (the PidFd api doesn't appear to include a function for getting the pid ☹️), that should save some space.

Maybe we can already enable it under #[feature(linux_pidfd)] rust feature flag for nightly builds?

👍 Possibly, we did have a nightly feature flags in the past.

[Skgland]

I think process_wait/2 should do all the cleanup.

That would mean you couldn't release the process reference without waiting for the process to exit. process_release/1 is there to release the process object when you don't want to wait for the process to exit, you wouldn't normally use it.

Actually, I think you were right. I forgot that process_release/1 waits for the process to exit, so what I said was incorrect.

[danilp-id]

So, we should:

1. Make it so that process_release/1 just releases the handle without waiting for the process to finish.
2. Add release/1 option to process_wait/3 and make it release(true) by default. Then process_release/1 should not be called after process_wait/3.

Now that I think about it, does one also need to close piped IO streams? Should process_release/1 and process_wait/2,3 close such streams automatically?

[Skgland]

So, we should:

1. Make it so that process_release/1 just releases the handle without waiting for the process to finish.

I was going to make it process_release(P) :- process_wait(P, _).
This would match the behavior of swi.

The problem with not waiting on the process is that we would just free the prolog/rust object, but the OS would still keep its state around until we wait on the process. I am not aware of a way to tell the OS that we won't wait on a process after we have spawned it.
Otherwise we would leak zombie processes, which I don't think is desireable.

Though we already leak zombie processes for process_create/2 and process_create/3 without process(P)option.

Alternatively we could add a reap list that is regularly checked for exited processes,
processes created without the process(P) option and those released with process_release/1 would then be added to this list so that they are reaped at some point after they have exited.
Checking the reap list as part of process_create/3 would sound reasonable to me.

3. Add release/1 option to process_wait/3 and make it release(true) by default.

Working on it.

Then process_release/1 should not be called after process_wait/3.

Yes

Now that I think about it, does one also need to close piped IO streams? Should process_release/1 and process_wait/2,3 close such streams automatically?

No, I don't think that makes sense.
The stdio streams are separate from the process object.
If we still have handles to our end of those streams then the other end would simply be closed.
Writing should as such result in eof and reading might still be able to read buffered data.
In the case of process_release/1 not waiting on the process both sides could still be happily communicating even without accounting for a buffer.

[Skgland]

The tests I am trying to add are running into #3300 ☹️

[danilp-id]

The tests I am trying to add are running into #3300 ☹️

You can use bb_put and bb_get as a workaround!