Repository navigation
Fix ptrace race on Android tests - #209
Conversation
There was a problem hiding this comment.
Oh no... We had a mid-air collision on this too! And again -- I just merged this in before I saw yours!
https://github.com/rust-minidump/minidump-writer/blob/main/tests/ptrace_dumper.rs#L47
Sorry about both of these - My fault. I haven't really been communicating that I had to fix these things as part of making my process inspection changes work.
It's funny -- We both ran into some of the same issues with testing doing our work. Our changes aren't even that dissimilar lol.
It looks like you might have taken a less shotgun-y approach here though - Yours might be clearly better. What do you think? Fix the merge conflicts and still do this instead?
dce600e to
964b23c
Compare
You can't have multiple children attaching to the parent at the same time, this forces serialization of this part of the tests. I would often see failures on my Android emulator because of this. Funnily enough the issue wasn't triggered on desktop Linux because the tests run through `cargo run` which adds enough delay to avoid the race window. This replaces the previous approach that relied on each test explicitly taking out the lock.
964b23c to
9880eda
Compare
|
Sure! I do love that not only did we end up with the same solution, but we also had the same side-quest of getting the output sorted out! |
There was a problem hiding this comment.
Perfect 💯
Thanks for doing this -- My way was... significantly crappier than this lol.
The main goal of this PR is to avoid a race when running multiple tests in parallel where they all try to ptrace() the parent and block each other out. I threw in a few output enhancements that I had to do in order to diagnose the issue.