oboe icon indicating copy to clipboard operation
oboe copied to clipboard

Will oboe use raw error callback when the client release shared pointer?

Open flamme opened this issue 1 year ago • 4 comments

From code AudioStreamAAudio::internalErrorCallback, it will try to promote the weak pointer to a shared pointer when there is an error callback. When the shared pointer exists, it will use the shared pointer for the error callback. Otherwise, it uses the raw pointer for the error pointer. My question here is that will there be a case that the client release the shared pointer before the error callback promote from the cached weak pointer? If that is the case, it looks like there can be crash happen as the raw pointer is also invalid.

flamme avatar Apr 26 '24 19:04 flamme

Interesting point. I think that code was designed to handle the case where an app did not use the shared_ptr interface to open the stream. But that branch may also be taken if someone used the shared pointer but closed the stream from the app before the error callback occured!

philburk avatar Apr 27 '24 18:04 philburk

I tried to force a race condition by adding a 4 second sleep in internalErrorCallback() right before I promote the weak pointer to a strong pointer. Then I play a stream in TEST OUTPUT, unplugged the USB headset, then hit the CLOSE button.

But instead of taking the RAW pointer branch it too this branch:

} else if (stream != oboeStream->getUnderlyingStream()) {
    LOGW("%s() stream already closed or closing", __func__); 

That is because the close() call nulled out the underlying stream.

We should write a test that verifies that the raw ptr branch is only taken when the app does not use the shared_ptr when opening a stream. Then we can maybe remove the API for creating a stream using anything other than the shared_ptr.

philburk avatar Apr 30 '24 23:04 philburk

I took another look into the framework's code. And I noticed that we had the protection from framework side that it only called the error callback from the data callback thread. In that case, this should be safe if the client call close before freeing the shared pointer as that the stream can only be closed after joining the data callback thread.

It is not able to reproduce such a crash as the code is pretty well written for OboeTester. Looking into OboeTester's code, it closes the stream before freeing the shared pointer. However, I was able to cause a crash by removing the close. Of course, for causing a crash, the sleep in the oboe error callback is also added in my local code to simulate a race condition.

flamme avatar May 01 '24 23:05 flamme

it only called the error callback from the data callback thread.

Is that true for both MMAP and Legacy streams?

philburk avatar Jun 10 '24 23:06 philburk