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 stack overflow on audio change #285

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

gridbugs
Copy link
Contributor

The types Frame and FrameData are mutually recursive, and the
incidental linked lists that can be formed as a result can be long (at
least in the order of thousands of elements). As a result, when a frame
is deallocated, rust appears to recursively call drop_in_place down
the list, causing stack overflows for long lists.

Addresses #283

This problem is hard to reproduce. I've tried constructing toy programs to no avail. The only way I've managed to trigger it is running https://github.com/stevebob/slime99/ graphical or ansi-terminal crates with the --audio flag (audio is disabled by default because of exactly this problem). Start a new game and leave it on the first floor for ~5 minutes, then walk down the stairs. The track will switch and sometimes there will be a stack overflow on the "rodio audio processing" thread.

I was able to reliably reproduce the problem last night, but I'm trying to again now (after changing nothing) and it's not happening. Something something phase of the moon. In other words, I'm not satisfied with my testing of this patch thus far. I'm opening this PR now to start a discussion. Is there a way to construct a long chain of Frame->FrameData->Frame->...? I think doing so would reliably trigger this problem.

@gridbugs gridbugs force-pushed the fix-frame-drop-stack-overflow branch from 2ea902b to 99c0c36 Compare April 22, 2020 13:11
@gridbugs
Copy link
Contributor Author

Ok I managed to reproduce it as per the above. My goal is to determine a correlation between the length of Frame->FrameData chain, and stack overflows. Then demonstrate that after my change, chains of problematic lengths no longer cause stack overflows.

I added the following right at the start of FrameData's drop:

    fn drop(&mut self) {
        let mut count = 0;
        let mut cursor = self.next.lock().unwrap().clone();
        loop {
            if let Frame::Data(data) = &*cursor.clone() {
                let next = data.next.lock().unwrap();
                cursor = next.clone();
            } else {
                break;
            }
            count += 1;
        }
        println!("count: {}", count);
        return;
       ...

...to measure the length of the chain. As an added bonus, the above code runs every time one of the nested Frames is dealloc'd, so I can see how far it got at the point where it crashed.

No crash:

  • 5645
  • 5530

Crash:

  • 12136 (crashed with 4476 to go (7760 complete))
  • 9853 (crashed with 2193 to go (also 7760 complete))
  • 12342 (crashed with 4682 to go (7760 complete))

Then I got rid of the return statement at the end of the block above, so the rest of drop can run but it still prints out the count. I saw a case where a chain of 11681 was dealoc'd, and it didn't stack overflow. Ran it again with 9126 chain length, and still no crash.

src/source/buffered.rs Outdated Show resolved Hide resolved
@gridbugs gridbugs changed the title Implement Drop for FrameData Fix stack overflow on audio change May 1, 2020
@gridbugs gridbugs force-pushed the fix-frame-drop-stack-overflow branch from 0650ba2 to a68f146 Compare August 2, 2020 23:17
The types `Frame` and `FrameData` are mutually recursive, and the
incidental linked lists that can be formed as a result can be long (at
least in the order of thousands of elements). As a result, when a frame
is deallocated, rust appears to recursively call `drop_in_place` down
the list, causing stack overflows for long lists.
@gridbugs gridbugs force-pushed the fix-frame-drop-stack-overflow branch from a68f146 to b1606ee Compare August 2, 2020 23:20
@gridbugs
Copy link
Contributor Author

gridbugs commented Aug 2, 2020

Squashed fix up commits and rebased onto the tip of master

@gridbugs
Copy link
Contributor Author

@nicokoch any thoughts on whether this change is ok to merge? Would you like me to rebase it onto master?

@mathstuf
Copy link
Contributor

Bump?

Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

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

Ok except for detail. Sorry for the delay.

// the `next` field will contain a `Frame::End`, or an `Arc` with additional references,
// so the depth of recursive drops will be bounded.
loop {
if let Ok(arc_next) = self.next.get_mut() {
Copy link
Member

Choose a reason for hiding this comment

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

This can be turned into a while let, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

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

This has sat unreviewed for more than a year. I'm merging despite the small issue. Subsequent PRs for improvement are welcome!

@est31 est31 merged commit 806ecaa into RustAudio:master Jan 12, 2022
@gridbugs
Copy link
Contributor Author

Thanks for the merge @est31! I'll make the change you suggested in a subsequent PR.

@gridbugs gridbugs mentioned this pull request Jan 13, 2022
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.

4 participants