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

Trap earlier when a Thread instance is re-used #3862

Merged
merged 3 commits into from Mar 14, 2017

Conversation

Projects
None yet
8 participants
@c1728p9
Contributor

c1728p9 commented Mar 1, 2017

Calling Thread::start multiple times leads to undefined behavior since the Thread class was not designed to handle this. Add an assert to trap immediately if Thread::start is called a second time to prevent this behavior.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Mar 1, 2017

/morph test

rtos/Thread.h Outdated
@@ -347,6 +347,7 @@ class Thread {
bool _dynamic_stack;
Semaphore _join_sem;
Mutex _mutex;
bool _finished;

This comment has been minimized.

@geky

geky Mar 1, 2017

Member

Random thought: Should we be ifdeffing these variables out or something for non-debug builds? Otherwise they are just wasting space.

This comment has been minimized.

@c1728p9

c1728p9 Mar 1, 2017

Contributor

Na, its just one byte.

This comment has been minimized.

@c1728p9

c1728p9 Mar 1, 2017

Contributor

I needed to update the PR anyway so I re-ordered this so the size stays the same 😲.

@geky

geky approved these changes Mar 1, 2017

This looks reasonable if we only want a thread to be startable once 👍

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 1, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1611

Test failed!

@c1728p9 c1728p9 force-pushed the c1728p9:thread_start_assert branch Mar 1, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Mar 1, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 1, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1612

Test failed!

@0xc0170

0xc0170 approved these changes Mar 2, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 2, 2017

/morph test

@0xc0170 0xc0170 added the needs: CI label Mar 2, 2017

@pan-

Could you update the documentation to reflect these changes and precise that it is not allowed to start a terminated thread ?

It is not indicated in the documentation and without this precision it is easy to guess that it is possible to reuse a terminated thread.

The API is also inconsistent because there is no way to differentiate a thread not started from a thread terminated.

rtos::Thread t; 
assert(t.get_state() == Thread::Deleted); 

t.start(...)
assert(t.get_state() != Thread::Deleted); 

t.join(); // || t.terminate()
assert(t.get_state() == Thread::Deleted); 

As a result it is impossible to start a Thread safely outside from its declaration site:

bool exec_on_thread(Thread& t) { 
    // might trap ... no way to avoid it by testing the state
    t.start(....);
}

Last point, is it necessary to trap. Thread::start was introduced to catch failure at user level and now an assertion for a non testable condition is introduced, I'm not sure to understand the logic.

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 2, 2017

I believe for the second run one ARCH_PRO and one HEXIWEAR had been knocked offline as the issue persisted in other CI runs. I have reset them so hopefully the same problem won't affect this run.

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 2, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1616

All builds and test passed!

@tim-nordell-nimbelink

This comment has been minimized.

tim-nordell-nimbelink commented Mar 2, 2017

I agree with @pan- that the documentation for the class should be updated to reflect this. It should be mentioned within the start() function documentation (that it cannot be restarted even if the thread has been terminated), and possibly in the overview of the class above the definition of the class as well.

@c1728p9 c1728p9 force-pushed the c1728p9:thread_start_assert branch Mar 3, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Mar 3, 2017

Since start returns an error code currently I updated this PR return an error rather than asserting. I also added a way to distinguish a thread that hasn't been started from a thread that has terminated.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Mar 3, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 3, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1622

Test failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 6, 2017

@pan- Please review again

@pan-

This comment has been minimized.

Member

pan- commented Mar 6, 2017

@0xc0170 Thanks to @c1728p9 the API is now consistent but the behavior (threads can be only started once) is still not documented.

I also wonder if it wouldn't be a good thing to enforce that instances have well defined and unique identity by forbidding copy constructor and copy assignment operator.

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Mar 6, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Mar 6, 2017

@pan- the thread being started only once is documented here

@pan-

This comment has been minimized.

Member

pan- commented Mar 6, 2017

@c1728p9 My bad, I missed that line 😞 .
LGTM

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Mar 6, 2017

/morph test

rtos/Thread.h Outdated
@@ -197,6 +197,7 @@ class Thread {
/** Starts a thread executing the specified function.
@param task function to be executed by this thread.
@return status code that indicates the execution status of the function.
@note a thread can only be stared once

This comment has been minimized.

@tim-nordell-nimbelink

tim-nordell-nimbelink Mar 6, 2017

Should be "started once" and not "stared once".

This comment has been minimized.

@c1728p9

c1728p9 Mar 6, 2017

Contributor

Thanks for catching this @tim-nordell-nimbelink

rtos: Return an error when a Thread is re-used
Calling Thread::start multiple times leads to undefined behavior since
the Thread class was not designed to handle being restarted.  Return an
error code if Thread::start is called a second time to prevent this
behavior.

c1728p9 added some commits Mar 3, 2017

rtos: Add Inactive return to thread get state
If a thread hasn't been started return Inactive as the status when
Thread::get_state() is called.
rtos: Prevent Thread class from being copied
Make the copy constructor and assignment operator private to prevent
them from being used.

@c1728p9 c1728p9 force-pushed the c1728p9:thread_start_assert branch to ab4da40 Mar 6, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Mar 6, 2017

/morph test

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Mar 6, 2017

@pan- I make the copy constructor and assignment operator private.

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 6, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 6, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1646

All builds and test passed!

@adbridge adbridge merged commit 88fb819 into ARMmbed:master Mar 14, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has started
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment