Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve subprocess-related docstrings and functions #1562

Merged
merged 4 commits into from
Mar 4, 2025

Conversation

pyrmont
Copy link
Contributor

@pyrmont pyrmont commented Mar 2, 2025

This PR simplifies, while aiming to preserve the clarity of, the docstrings for various subprocess-related functions in os.c. It also tidies up some unnecessary return statements and Windows-related compiler conditional directives (i.e. #ifdef) .

Background

In #1394, @amano-kenji added detail to the docstrings of a number of functions relating to subprocesses. This improved the explanations of behaviour relating to subprocesses that had been discussed in #1386. In short, it wasn't always clear how to avoid zombie processes and deadlock when using subprocesses. The docstrings were expanded to provide more detail on these points.

Implementation

This PR attempts to simplify the wording of the docstrings by using consistent terminology. Small grammatical errors that may have impacted readability are also fixed.

@bakpakin
Copy link
Member

bakpakin commented Mar 2, 2025

Note that an error can occur even if os/proc-wait is apparently canceled (e.g. with ev/with-deadline) due to the way os/proc-wait is implemented.

This is a bug, no?

@bakpakin bakpakin added the bug This is not expected behavior, and needs fixing label Mar 2, 2025
@bakpakin
Copy link
Member

bakpakin commented Mar 3, 2025

Otherwise, LGTM. The bit about the extra error should be remove, and then this can be merged.

@pyrmont
Copy link
Contributor Author

pyrmont commented Mar 3, 2025

@bakpakin wrote:

This is a bug, no?

It might be more correct to call it a limitation inherent to the implementation. If you take your new test and add a new call to os/proc-wait after the first assertion like so:

# Cancel os/proc-wait with ev/deadline 2
(let [p (os/spawn [;run janet "-e" "(os/sleep 0.1)"] :p)]
  (var terminated-normally false)
  (assert-error "deadline expired"
                (ev/with-deadline 0.05
                  (os/proc-wait p)
                  (print "uhoh")
                  (set terminated-normally true)))
+ (os/proc-wait p)
  (assert (not terminated-normally) "early termination failure 2")
  (ev/sleep 0.15)
  (assert (not terminated-normally) "early termination failure 3"))

this will still raise an error. I believe this is because while the fiber running os/proc-wait has been cancelled, p still has a flag set for JANET_PROC_WAITING and os_proc_wait_impl check this and panics:

janet/src/core/os.c

Lines 596 to 599 in e0a0e2e

os_proc_wait_impl(JanetProc *proc) {
if (proc->flags & (JANET_PROC_WAITED | JANET_PROC_WAITING)) {
janet_panicf("cannot wait twice on a process");
}

I don’t know if there’s a way to address this.

Regardless, on further reflection, I think the deadline stuff is more detail than is necessary for the docstring so I’ve removed in the latest commit (together with a few further clean-ups).

@bakpakin bakpakin merged commit 9538b8a into janet-lang:master Mar 4, 2025
13 checks passed
@pyrmont pyrmont deleted the bugfix.subprocess-cleanup branch March 4, 2025 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This is not expected behavior, and needs fixing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants