Skip to content

Fix wait_with_pipe() so it waits for last child and uses exit status #83

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

Merged
merged 6 commits into from
Aug 3, 2025

Conversation

delan
Copy link
Contributor

@delan delan commented Aug 2, 2025

unlike all of the other wait methods in FunChildren and CmdChildren, wait_with_pipe() does not actually wait for the last child in the pipeline to exit. it kills the last child when the provided function returns, and ignores that child’s exit status. killing the last child is potentially useful, to avoid deadlock if the caller fails to fully read stdout, but ignoring the last child’s exit status seems like a bug.

this patch fixes that bug by making wait_with_pipe() wait for the last child and use its exit status. if the child can’t exit because it has unread output, we automatically read and discard that output and wait again, until the child exits.

this is a breaking change for two reasons:

  • f now takes a &mut Box<dyn Read>, not a Box<dyn Read> — though depending on the code provided in f, this may only result in a new #[warn(unused_mut)] warning
  • commands like yes or sh -c "yes >&2 &" now indefinitely block the return of wait_with_pipe() — though this is consistent with the behaviour of all the other wait methods

@tao-guo
Copy link
Collaborator

tao-guo commented Aug 3, 2025

I am ok with the behavior change and to make f to take &mut. It was actually &mut when I first implemented. Just resolve the conflicts and I am happy to merge the improvements. Thanks for taking time to fix these subtle issues.

@delan
Copy link
Contributor Author

delan commented Aug 3, 2025

working on it, just need to check how it interacts with another feature that will depend on this…

@delan delan marked this pull request as draft August 3, 2025 05:27
@delan delan force-pushed the fix-wait-with-pipe branch from 4e0e99e to a0bb123 Compare August 3, 2025 09:59
@delan delan changed the title RFC: how to fix wait_with_pipe()? Fix wait_with_pipe() so it waits for last child and uses exit status Aug 3, 2025
@delan delan marked this pull request as ready for review August 3, 2025 10:15
@delan
Copy link
Contributor Author

delan commented Aug 3, 2025

ok, i think we’re good to go! i tested this with a lot of cases to understand exactly how the behaviour would change:

  • sleep 1
  • sh -c "sleep 1 &"
  • sh -c "sleep 1; echo foo &"
  • sh -c "dd if=/dev/zero status=progress &"
  • sh -c "dd if=/dev/zero status=progress >&2 &"
  • sh -c "dd if=/dev/zero of=/dev/null &"
  • yes
  • sh -c "yes >&2 &"
  • those cases with empty f
  • those cases with a small sleep in f
  • those cases with a blocking read in f
  • plus forgetting StderrThread without joining (to avoid blocking on grandchildren)
  • plus using non-blocking stdout reads (to avoid blocking on grandchildren)
  • plus using it to log stdout in run_cmd!() (which is how i found the bug in ff05adb)

my conclusions for now are:

  • while there are still some breaking changes, it’s not really possible to fix this bug without breaking changes
  • while there are still some cases where the program might block forever, this is also true for the rest of the API

@tao-guo tao-guo merged commit 3bbf340 into rust-shell-script:master Aug 3, 2025
1 check passed
@delan delan deleted the fix-wait-with-pipe branch August 3, 2025 15:09
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.

2 participants