0.2.3 OSX Crash on exit

[cjcliffe]

Exception Type: EXC_CRASH (SIGABRT)
Exception Codes: 0x0000000000000000, 0x0000000000000000
Exception Note: EXC_CORPSE_NOTIFY

Application Specific Information:
Assertion failed: (e == 0), function ~recursive_mutex, file /BuildRoot/Library/Caches/com.apple.xbs/Sources/libcxx/libcxx-120.1/src/mutex.cpp, line 82.

[vsonnier]

@cjcliffe I can't help you much here, because I don't have a Mac.
Still, I noticed an Apple-specific call to AudioThread::deviceCleanup() on exit.

What is the purpose of this call ?

It apparently calls a terminate() on AudioThread* referenced in a static std::map<int, AudioThread *> AudioThread::deviceController.

Since all AudioThread* are cleaned up on DemodulatorInstance death which is automatic, we should not touch std::map<int, AudioThread *> AudioThread::deviceController at all at application termination because it may contain dangling pointers, referencing already de-allocated AudioThread *.

Only my 2 euro-cents, here :)

[cjcliffe]

@vsonnier I figured you wouldn't be able to test but might be able to weigh in on recursive_mutex issues. Will post here as I uncover stuff later.

[vsonnier]

In the commit d8ac955 on master I've done some adjustments on BookmarkMgr where recursive_mutex are used:

• some unprotected calls were fixed,
• some others didn't need locks at all so I've removed them,
• Changed some calls to return copies instead of references which is better in multithread.

I don't know if it is going to affect the present problem, though.

The 3 others classes using recursive_mutex are the DemodulatorInstance / Mgr and AudioThread, and it really seems to be needed there.

[vsonnier]

I tried in 36224de to rework the AudioThread by understanding the class better, putting a lot of comments as well.
When attempting to cleanup the deviceController list by deviceCleanup() at termination, (as we should) the call hang here on Windows because apparently the matching thread is seemingly already joined and deallocated....
So I guess it is to be applied only on MacOS as you did before.

[cjcliffe]

@vsonnier bookmarkmgr d8ac955 updates seemed fine here on the macbook; but it wasn't crashing here on exit either way -- just my work machine appears to do that.

Latest update at 36224de now appears to hang on exit on the macbook and requires a forced quit if any demod was created.

[vsonnier]

@cjcliffe Ah Progress ! When More is Less !
In commit fc1c1c3 I rollbacked to a deviceCleanup() doing only a terminate, not join() that hang both our machines.

Still I think the problem really lies in the deviceController list, where it seems the AudioThread thread is apparently already terminated.
Indeed, we never goes through this part of run() at termination or otherwise:

CubicSDR/src/audio/AudioThread.cpp

Line 527 in fc1c1c3

try {

that concerns controller threads and supposed to close the corresponding device proprerly.

[vsonnier]

Right, I think I've found the problem preventing the deviceController threads to be joined properly: because of deadlocks. Go figure. How surprising.
Well I think everything looks fine after the last commit f2de1dd.
Tell me how it behaves for you.

[cjcliffe]

@vsonnier latest commit seems to be working fine on the macbook; will let you know how it goes on the iMac

[cjcliffe]

@vsonnier looks good on the iMac too; no more crashes on exit or hangups with the latest commit -- thanks! I wasn't able to figure out how to properly trace a thread issue like this from XCode.

Going to update the current OSX release with the bugfixed version.

[vsonnier]

Well, there was no magic involed in finding these deadlocks, just a matter of putting breakpoints going step by step, and the help that join() on the deviceController threads was never completing.
So by interrupting the execution in debugger freezing the whole threads I eventually found a blocking point at some std::lock_guard. On Windows version it has a data member flag locked showing true.
Then walking on the suspended threads I found the other frozen std::lock_guard...
Then I swore a lot, because it was obvious :)