Skip to content

Add tryWait() to Child class#21

Open
slomkowski wants to merge 2 commits into
kgit2:mainfrom
slomkowski:main
Open

Add tryWait() to Child class#21
slomkowski wants to merge 2 commits into
kgit2:mainfrom
slomkowski:main

Conversation

@slomkowski

Copy link
Copy Markdown

I added an implementation of tryWait() method to Child class. It more-or-less follows the semantics of try_wait() from Rust:

  • if the process is still running, it returns null
  • otherwise, it returns the exit status

Please let me know if the coding style is appropriate for your project.

@BppleMan BppleMan requested review from BppleMan and Copilot May 7, 2025 04:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a new tryWait() method to the Child class, providing a non-blocking way to check on the process exit status following a Rust-inspired design.

  • Adds tryWait() functionality to both native and JVM implementations
  • Introduces a helper conversion function (fromOptional) in the native extensions
  • Updates tests and FFI bindings to support the new API

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/nativeMain/kotlin/com/kgit2/kommand/process/Child.native.kt Adds tryWait() using try_wait_child and conversion via fromOptional
src/nativeMain/kotlin/com/kgit2/kommand/Extension.kt Implements Int.fromOptional helper for handling process state
src/jvmMain/kotlin/com/kgit2/kommand/process/Child.jvm.kt Implements tryWait() with a zero-timeout waitFor call
src/commonTest/kotlin/com/kgit2/kommand/ChildTest.kt Introduces tests to validate tryWait() behavior
src/commonMain/kotlin/com/kgit2/kommand/process/Child.kt Declares tryWait() in the common interface
kommand-core/src/result.rs Adjusts IntResult conversion to incorporate optional exit statuses
kommand-core/src/process/child.rs Adds FFI function for try_wait_child
kommand-core/kommand_core.h Exposes the try_wait_child function in the C header

Comment thread src/jvmMain/kotlin/com/kgit2/kommand/process/Child.jvm.kt Outdated
Comment thread src/nativeMain/kotlin/com/kgit2/kommand/process/Child.native.kt Outdated
Comment on lines +120 to +123
Ok(status) => Ok(Some(status)),
Err(e) => Err(e),
}
.into()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Ok(status) => Ok(Some(status)),
Err(e) => Err(e),
}
.into()
value.map_or_else(Err, |status| Ok(Some(status))).into()

Comment on lines +132 to 134
ok: -2,
err: into_cstring("Application still running"),
error_type: ErrorType::None,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add new ErrorType variant for "Application still running"

}

fun Int.Companion.fromOptional(result: CValue<kommand_core.IntResult>): Int? = memScoped {
return@memScoped if (result.ptr.pointed.ok == -2) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return@memScoped if (result.ptr.pointed.ok == -2) {
return@memScoped if (result.ptr.pointed.error_type == /* what's for application still running */) {

actual fun tryWait(): Int? {
return when (Int.fromOptional(try_wait_child(inner))) {
null -> null
else -> wait()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the rust try_wait example, when an error occurs in try_wait, it should be up to the caller to choose whether to do a blocking wait, and it seems that it should not be handled by default in the library.

use std::process::Command;

let mut child = Command::new("ls").spawn().unwrap();

match child.try_wait() {
    Ok(Some(status)) => println!("exited with: {status}"),
    Ok(None) => {
        println!("status not ready yet, let's really wait");
        let res = child.wait();
        println!("result: {res:?}");
    }
    Err(e) => println!("error attempting to wait: {e}"),
}

@Throws(KommandException::class)
actual fun tryWait(): Int? {
return when (process.waitFor(0, TimeUnit.MICROSECONDS)) {
true -> wait()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as Child.native.kt


@Throws(KommandException::class)
actual fun tryWait(): Int? {
return when (process.waitFor(0, TimeUnit.MICROSECONDS)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use

/**
  * Returns the exit value for the process.
  *
  * @return the exit value of the process represented by this
  *         {@code Process} object.  By convention, the value
  *         {@code 0} indicates normal termination.
  * @throws IllegalThreadStateException if the process represented
  *         by this {@code Process} object has not yet terminated
  */
public abstract int exitValue();

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exitValue throws an exception when the process is not finished (which we should catch), waitFor seems a bit cleaner to me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be a special test mechanism for tryWait, such as executing a process lasting 5 seconds through Command, and calling tryWait periodically with 1 second, and you should expect 5 nulls and 1 ok.

@BppleMan BppleMan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your code contribution, this is a cool solution, but there are still some details to be confirmed.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants