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.

Sign up for GitHub

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

Jump to bottom

Try fixing random hangs in CI due to races #2615

Closed
wants to merge 2 commits into from
Closed

Try fixing random hangs in CI due to races #2615

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e99152
Try implementing solution pointed by @yihuaf in https://github.com/co…
YJDoc2 Jan 4, 2024
284a4d5
Update comment as per suggestion
YJDoc2 Jan 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions crates/libcontainer/src/process/fork.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,24 @@ fn clone(cb: CloneCb, flags: u64, exit_signal: Option<u64>) -> Result<Pid, Clone
// arg is actually a raw pointer to the Box closure. so here, we re-box the
// pointer back into a box closure so the main takes ownership of the
// memory. Then we can call the closure.

// The reason for test/non-test split via cfg is that after forking,
// the malloc and free call (from Box) can race and hang up. This is seen only in
// CI tests due to tests being run in parallel via cargo, so leaking memory by leaking
YJDoc2 marked this conversation as resolved.
Show resolved Hide resolved
// box to prevent it, only in test config. See https://github.com/containers/youki/issues/2144
// and https://github.com/containers/youki/issues/2144#issuecomment-1624844755
// for more detailed analysis
#[cfg(not(test))]
extern "C" fn main(data: *mut libc::c_void) -> libc::c_int {
unsafe { Box::from_raw(data as *mut CloneCb)() }
}
#[cfg(test)]
extern "C" fn main(data: *mut libc::c_void) -> libc::c_int {
let mut func = unsafe { Box::from_raw(data as *mut CloneCb) };
let ret = func();
Box::into_raw(func);
ret
}
Comment on lines +214 to +224
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[cfg(not(test))]
extern "C" fn main(data: *mut libc::c_void) -> libc::c_int {
unsafe { Box::from_raw(data as *mut CloneCb)() }
}
#[cfg(test)]
extern "C" fn main(data: *mut libc::c_void) -> libc::c_int {
let mut func = unsafe { Box::from_raw(data as *mut CloneCb) };
let ret = func();
Box::into_raw(func);
ret
}
extern "C" fn main_impl(data: *mut libc::c_void) -> libc::c_int {
unsafe { Box::from_raw(data as *mut CloneCb)() }
}
#[cfg(not(test))]
extern "C" fn main(data: *mut libc::c_void) -> libc::c_int {
main_impl(data)
}
#[cfg(test)]
extern "C" fn main(data: *mut libc::c_void) -> libc::c_int {
let mut func = main_impl(data);
let ret = func();
Box::into_raw(func);
ret
}

Copy link
Member

Choose a reason for hiding this comment

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

We should also use #[cfg(any(target_env = "musl"))]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey,

  1. We can extract the main logic in a main_impl function, but it will need to leak the box or return the box which then the caller will have to leak it. If we do it as you have suggested, i.e. calling the boxed function and returning the result, we still do the free call when main_impl returns and run into same potential race condition.
  2. As per the last comment on that issues, runwasi was running into same hanging-up issue when running with libc. As far as I understand, the root cause if doing malloc/free after the fork, which is supposed to be undefined behavior. As this only affect in tests, I had added cfg test, and kept rest of the targets to the normal implementation. As per yihuaf 's comment, which I have linked in the desc, the hand issue should only be observed in tests, not in actual usage, so I feel only modifying the test target with cfg is better than modifying the whole musl compilation.

wdyt?


// The nix::sched::clone wrapper doesn't provide the right interface. Using
// the clone syscall is one of the rare cases where we don't want rust to
Expand Down