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

fix: rust unit test "Text file busy" #996

Merged
merged 2 commits into from
Feb 13, 2024
Merged

fix: rust unit test "Text file busy" #996

merged 2 commits into from
Feb 13, 2024

Conversation

ysndr
Copy link
Contributor

@ysndr ysndr commented Feb 13, 2024

Resolve issues with rust unit tests in models::container_builder::tests::*.

The container builder tests spuriously failed with Text file busy:

thread 'models::container_builder::tests::test_writes_output_to_writer' panicked at flox-rust-sdk/src/models/container_builder.rs:81:54:
called `Result::unwrap()` on an `Err` value: CallContainerBuilder(Os { code: 26, kind: ExecutableFileBusy, message: "Text file busy" })
stack backtrace:
   0: rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::result::unwrap_failed
   3: core::result::Result<T,E>::unwrap
             at /build/rustc-1.73.0-src/library/core/src/result.rs:1077:23
   4: flox_rust_sdk::models::container_builder::tests::test_writes_output_to_writer
             at ./src/models/container_builder.rs:81:9
   5: flox_rust_sdk::models::container_builder::tests::test_writes_output_to_writer::{{closure}}
             at ./src/models/container_builder.rs:76:39
   6: core::ops::function::FnOnce::call_once
             at /build/rustc-1.73.0-src/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Text file busy (26) may occur when executing a script
that is has been written to immediately prior.
This is a known bug in rust (and allegedly other languages)
which is tracked in rust-lang/rust#114554.

We typically see this error in tests, where we write a new container builder script
and immediately execute it within the same process:

https://github.com/flox/flox/blob/main/cli/flox-rust-sdk/src/models/container_builder.rs#L76-L83

In production use, this should not be a problem as the script will be written
by a different process, i.e. pkgdb.

@ysndr ysndr force-pushed the fix/flaky/rust-test branch 3 times, most recently from 58dd004 to d1e95d3 Compare February 13, 2024 12:31
@ysndr ysndr marked this pull request as ready for review February 13, 2024 12:40
Copy link
Contributor

@aakropotkin aakropotkin left a comment

Choose a reason for hiding this comment

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

Optional change request. I can do a quick rereview if you do make the change.

cli/flox-rust-sdk/src/models/container_builder.rs Outdated Show resolved Hide resolved
@ysndr ysndr enabled auto-merge February 13, 2024 12:53
@ysndr ysndr disabled auto-merge February 13, 2024 12:54
Copy link
Contributor

@mkenigs mkenigs left a comment

Choose a reason for hiding this comment

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

I saw this flake after 500 runs, which is a lot better. If we decide that's too much at any point, we could use a mutex for any tests that are forking, or run any tests that fork in a single thread

@@ -59,6 +59,17 @@ mod tests {

use super::*;

/// OS error 26 is "Text file busy",
/// which can happen when executing a script
/// that is has been written to immediately before.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the race is caused by threading + forking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think its due to forking, at least not if this can be attributed to rust-lang/rust#114554.

Even if cargo test runs by forking processes (which I'm not certain about either), each process will write to files in different tempdirs.

Copy link
Contributor

Choose a reason for hiding this comment

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

> This is due to the following sequence of events occuring:
> 
>     1. The main thread opens the file descriptor for writing into `/tmp/executable.sh`.
> 
>     2. The spawned thread calls `fork(2)`, inheriting the file descriptor that has write access to `/tmp/executable.sh`.
> 
>     3. The main thread closes its copy of the file descriptor — but note that it is **not** closed in the child process.
> 
>     4. The main thread then attempts to execute the file. However, because there exists a process with a file descriptor open that has write access to the file, it fails with ETXTBSY and the process panics.

Copy link
Contributor

@mkenigs mkenigs Feb 13, 2024

Choose a reason for hiding this comment

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

cargo test is threading, Command is forking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Command::spawn == fork() -> exec() that fork, I see..

I think it's clear enough for now, for more detail there is the upstream issue linked as well

cli/flox-rust-sdk/src/models/container_builder.rs Outdated Show resolved Hide resolved
cli/flox-rust-sdk/src/models/container_builder.rs Outdated Show resolved Hide resolved
Co-authored-by: Matthew Kenigsberg <matthew@floxdev.com>
@ysndr ysndr added this pull request to the merge queue Feb 13, 2024
Merged via the queue into main with commit 9c1d5e6 Feb 13, 2024
14 checks passed
@ysndr ysndr deleted the fix/flaky/rust-test branch February 13, 2024 18:44
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.

None yet

3 participants