HydrogenAudio

Hosted Forums => foobar2000 => Development - (fb2k) => Topic started by: MordredKLB on 2021-05-11 20:41:44

Title: Foobar.exe's abort_callback in simple_thread_task causing crashes
Post by: MordredKLB on 2021-05-11 20:41:44
So I and my users are encountering an intermittent crash that appears to be some kind of timing/race condition thing when aborting searches in my foo_musicbrainz component.

I first query musicbrainz to get a list of matching results, and then I spawn a number of simple_thread_task (https://github.com/kbuffington/foo_musicbrainz/blob/master/src/request_thread.cpp#L149)'s, one for each result which does the individual queries. Every one of these threads is spawned from a threaded_process_callback (https://github.com/kbuffington/foo_musicbrainz/blob/master/src/request_thread.cpp#L93) which gets a abort_callback& from foobar and passes it down to all the simple_thread_tasks.

Occasionally, when a user aborts a search I will get access_violations (which crash) attempting to do anything with the abort_callback (i.e. abort->is_aborting()). Unfortunately, all VS2019 will tell me is that abort_callback comes from foobar.exe and provides no useful information, so I can't tell if the ptr is now garbage, or if some value inside the object has been corrupted.

It's real intermittent so sometimes I can abort 20 times in a row and they all exit gracefully, and then other times, the first abort causes it to happen.

I'm encountering this using 1.6.5 and SDK 2021-02-23. Users have reported it in 1.6.6b7 as well.

There is certainly a chance that I'm doing something wrong, but it feels like this is an issue with foobar. @Peter, if this is something you think is worth investigating, you can get the latest beta here (https://hydrogenaud.io/index.php?topic=70623.msg997452#msg997452) with the corresponding pdb.
Title: Re: Foobar.exe's abort_callback in simple_thread_task causing crashes
Post by: snotlicker on 2021-05-11 21:06:27
You need to remember that simple_thread_task and the thread pool implementation you're using has nothing to do with the SDK/Peter. It was never part of foo_musicbrainz when I maintained it so I assume you've copied it from foo_enhanced_playcount. Remember I told you the original implementation was from WSH panel mod. It's always been rock solid but of course aborting tasks was never attempted in any code it's been used for before.
Title: Re: Foobar.exe's abort_callback in simple_thread_task causing crashes
Post by: MordredKLB on 2021-05-11 22:08:55
You need to remember that simple_thread_task and the thread pool implementation you're using has nothing to do with the SDK/Peter. It was never part of foo_musicbrainz when I maintained it so I assume you've copied it from foo_enhanced_playcount. Remember I told you the original implementation was from WSH panel mod. It's always been rock solid but of course aborting tasks was never attempted in any code it's been used for before.
Crap, you're right. I was thinking simple_thread_task was SDK, but it's not. Maybe that changes things, maybe it doesn't though.

Every one of the threads has a reference to the same abort_callback value, and none of them alter it in any way (they can't because abort_callback does not contain any methods that allow for that). abort_callback's value is controlled from foobar.exe, completely outside my component. Doesn't make a ton of sense, but maybe there's something going on where one of the child threads is reading the value, at the same time the main UI thread is altering the value?

This seems unlikely because I would expect the reading to be atomic, and also because before my recent change the crash could take up to a second to manifest. When I first discovered the issue the child threads were always waiting up to 1s (to avoid hammering) before calling request->run_ex(url, abort) which was where the crash was happening. After adding some abort->is_aborting() calls in an effort to avoid the crash, it just moved the crash position to those.
Title: Re: Foobar.exe's abort_callback in simple_thread_task causing crashes
Post by: Peter on 2021-05-12 07:58:06
An abort_callback& that you get is ONLY VALID FOR THE DURATION OF THE CALL in which you got it.
You CANNOT make assumptions about its lifetime beyond that.
The same can be said for all objects passed by reference unless explicitly documented otherwise.

Passing it to other threads is OK - as long as you guarantee that these threads have exited by the time your method that got the abort_callback& returns.

A common practice in my current code is to pass std::shared_ptr<abort_callback_impl> to worker threads to avoid blocking waiting for them to finish. But you can't just turn an abort_callback& from calling code into that - create a new one and set it in your master thread when aborted by calling code.
Title: Re: Foobar.exe's abort_callback in simple_thread_task causing crashes
Post by: MordredKLB on 2021-05-12 08:56:50
Passing it to other threads is OK - as long as you guarantee that these threads have exited by the time your method that got the abort_callback& returns.
I was fairly confident I was doing exactly that, but you made me re-examine my code.

When I fire off my threads I sit and spin in the main threaded_process_callback waiting for them all to finish. However if I get an abort I stopped waiting for those threads to finish because the wait could have been several seconds (but is now essentially instant). abort_callback& was stale at that point and probably being overwritten while the threads were still finishing up processing and handling the abort_callback that was passed down to them.

Simple solution seems to be just enforcing main thread to wait for all the child threads to handle abort first, then exit. Problem appears to be solved. Thanks Peter!
SimplePortal 1.0.0 RC1 © 2008-2021