Two iterations of Thread start(), join() causes mbed_die()

[tim-nordell-nimbelink]

Description

• Type: Bug
• Priority: Major

---

Bug

meed-os sha:
2885c1b Merge pull request #3510 from ARMmbed/release-candidate

Expected behavior
A loop that does something akin to:

1. thread.start();
2. thread.join();

should continue indefinitely (where the thread starting function exited between start and join).

Actual behavior
mbed_die() is invoked due to the MBED_ASSERT(NULL == __tid) within the join() call. The function gets this far due to the semaphore count within "_join_sem" not being reset when the thread is restarted. (A count of 1 is left at the end of the ::join(), and at the next ::start() it does not reset this count back to 0.)

Steps to reproduce
Create a thread that exits immediately from a callback function, and invoke start() and join on against that twice.

Suggested fix

diff --git a/rtos/Thread.cpp b/rtos/Thread.cpp
index 61bfe964c..673f8c0c7 100644
--- a/rtos/Thread.cpp
+++ b/rtos/Thread.cpp
@@ -101,6 +101,8 @@ osStatus Thread::start(Callback<void()> task) {
         _mutex.unlock();
         _join_sem.release();
         return osErrorResource;
+    } else {
+        while(_join_sem.wait(0) != 0);
     }
 
     _mutex.unlock();

There are potential problems with this though, but not any more than were already there. If two threads asynchronously call join() and start() after the first join(), it could cause join() to pass the semaphore check, wait at the mutex, and then end in a state that it hits the same assertion this sample fix fixes. Thus the sample fix is only good if 1 thread is invoking the join() or start() calls. One way to further fix this would be to introduce one more mutex - one (already implemented) that protects setting/clearing of _tid and any other thread synchronization issues between the thread that starts the other thread and the other thread itself, and a second one (not implemented) that surrounds all transactions involving starting and stopping the thread (e.g. start(), join() and terminate()) to ensure only one thread at a time is invoking these.

[0xc0170]

cc @c1728p9 @geky

[geky]

@c1728p9?

The above patch looks reasonable to me if I am understanding the problem, though @c1728p9 is the architect behind the join behaviour.

[c1728p9]

The thread class was not designed to be started multiple times. There should be code to assert that this does not occur.

[tim-nordell-nimbelink]

If this is the case (unsupported), the documentation in the header file should be updated.

[c1728p9]

Hi @tim-nordell-nimbelink, that example only calls start just once. It does not call it in a loop.

[tim-nordell-nimbelink]

@c1728p9 Yeah, I realized that after I posted the clip from the documentation. (I was just glancing at it now.) Either way, if it is intentionally not supported, the second invocation of start() should probably assert() itself instead of the second join() call, and the documentation should be updated.

[c1728p9]

I agree. I'll create a PR to make it assert in the start.

[c1728p9]

Created #3862

[tim-nordell-nimbelink]

#3862 is merged in. Closing this.