Skip to content

Fix handling of ignore in wait() and wait_with_all() #82

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 5 commits into from
Aug 3, 2025

Conversation

delan
Copy link
Contributor

@delan delan commented Aug 2, 2025

the logic in CmdChildren and FunChildren is currently a bit organic and inconsistent. as a result, the following test cases (also added in this patch) return the wrong results:

assertion failed (false != true): spawn!(ignore false).unwrap().wait().is_ok() when pipefail is on
assertion failed (false != true): spawn!(ignore false).unwrap().wait().is_ok() when pipefail is off
assertion failed (false != true): spawn!(ignore true | false).unwrap().wait().is_ok() when pipefail is on
assertion failed (false != true): spawn!(ignore true | false).unwrap().wait().is_ok() when pipefail is off
assertion failed (false != true): spawn!(ignore false | true).unwrap().wait().is_ok() when pipefail is on
assertion failed (false != true): spawn!(ignore false | false).unwrap().wait().is_ok() when pipefail is on
assertion failed (false != true): spawn!(ignore false | false).unwrap().wait().is_ok() when pipefail is off
assertion failed (false != true): spawn_with_output!(ignore false).unwrap().wait_with_all().0.is_ok() when pipefail is on
assertion failed (false != true): spawn_with_output!(ignore false).unwrap().wait_with_all().0.is_ok() when pipefail is off
assertion failed (false != true): spawn_with_output!(ignore true | false).unwrap().wait_with_all().0.is_ok() when pipefail is on
assertion failed (false != true): spawn_with_output!(ignore true | false).unwrap().wait_with_all().0.is_ok() when pipefail is off
assertion failed (false != true): spawn_with_output!(ignore false | false).unwrap().wait_with_all().0.is_ok() when pipefail is on
assertion failed (false != true): spawn_with_output!(ignore false | false).unwrap().wait_with_all().0.is_ok() when pipefail is off

this patch fixes wait() and wait_with_all() to correctly handle ignore. it also adds a comprehensive test suite that verifies that all of the ways you can get a CmdResult or FunResult (“entry points”) correctly handle the ignore command and pipefail mode. note that:

  • wait_with_pipe() suffers from an unrelated and more complicated bug, so it’s not yet included in the test suite
  • set_pipefail() is not thread safe, so the new tests have to live in test_pipe() for now, otherwise both will often fail

@delan
Copy link
Contributor Author

delan commented Aug 2, 2025

to see the tests fail with the old code, check out this branch, then undo the changes to src/child.rs, e.g.

$ git remote add delan [email protected]:delan/rust_cmd_lib.git
$ git switch -c fix-ignore delan/fix-ignore
$ git restore -Ws @~ src/child.rs
$ cargo test

let other_children_res = Self::wait_children(&mut self.children);

self.ignore_error
.then_some(Ok(()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is precise and thanks for the clean up.

assert!(run_fun!(false | true).is_ok());
set_pipefail(true);
// test `ignore` command and pipefail mode
// FIXME: make set_pipefail() thread safe, then move this to a separate test_ignore_and_pipefail()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you thought about using sealed_test to make the tests work ? Or we can make the CI to run tests sequentially

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could do that, but i think it’s possible to fix the thread unsafety, and doing so would make the API more reliable. i’ll do that in a separate patch.

@tao-guo tao-guo merged commit c5c50e2 into rust-shell-script:master Aug 3, 2025
1 check passed
@delan delan deleted the fix-ignore branch August 3, 2025 03:46
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