Skip to content

Commit

Permalink
Tentative fix for initializer list thread race condition
Browse files Browse the repository at this point in the history
Tentative fix for this Duo report [here](https://2dimensions.slack.com/archives/C029X99PETE/p1692977259716259)
Make sure we initialize & start the thread so every other field is already initialized.

[Source](https://en.cppreference.com/w/cpp/language/constructor)
```
Initialization order

The order of member initializers in the list is irrelevant: the actual order of initialization is as follows:
1) If the constructor is for the most-derived class, virtual bases are initialized in the order in which they appear in depth-first left-to-right traversal of the base class declarations (left-to-right refers to the appearance in base-specifier lists).
2) Then, direct bases are initialized in left-to-right order as they appear in this class's base-specifier list.
3) Then, non-static data member are initialized in order of declaration in the class definition.
4) Finally, the body of the constructor is executed.
```

Also, a sort-of repro [here](https://alexanderhoughton.co.uk/blog/stdmutex-crash-on-initial-lock-interesting-thread-releated-bug-with-fix-included/)

Diffs=
4ff3d9cee Tentative fix for initializer list thread race condition (#5920)

Co-authored-by: Umberto Sonnino <[email protected]>
  • Loading branch information
umberto-sonnino and umberto-sonnino committed Aug 30, 2023
1 parent 01f752a commit 850bc8b
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 11 deletions.
2 changes: 1 addition & 1 deletion .rive_head
Original file line number Diff line number Diff line change
@@ -1 +1 @@
d35df04272e659d8244c776e9c4f558452888501
4ff3d9cee0a9a9acd7874ab6782015f7c11a661e
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class RiveActivityLifecycleTest {
// Close it down.
activityScenario.close()
// Background thread deallocates asynchronously.
waitUntil(500.milliseconds) { controller.refCount == 0 }
waitUntil(1500.milliseconds) { controller.refCount == 0 }
assertFalse(controller.isActive)
assertNull(controller.file)
assertNull(controller.activeArtboard)
Expand Down Expand Up @@ -79,7 +79,7 @@ class RiveActivityLifecycleTest {
// Close it down.
activityScenario.close()
// Background thread deallocates asynchronously.
waitUntil(500.milliseconds) { controller.refCount == 0 }
waitUntil(1500.milliseconds) { controller.refCount == 0 }
assertFalse(controller.isActive)
assertNull(controller.file)
assertNull(controller.activeArtboard)
Expand Down
4 changes: 3 additions & 1 deletion kotlin/src/main/cpp/clean_all.sh
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ popd

# PLS
pushd "$RIVE_RUNTIME_DIR"/../pls/out
make clean
if [[ -e Makefile ]]; then
make clean
fi
rm -rf ./android
rm -rf ./dependencies
rm -rf ./obj
Expand Down
15 changes: 8 additions & 7 deletions kotlin/src/main/cpp/include/helpers/worker_thread.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@ class WorkerThread
using WorkID = uint64_t;

WorkerThread(const char* name, Affinity affinity, const RendererType rendererType) :
m_RendererType(rendererType),
mName(name),
mAffinity(affinity),
mThread(std::thread([this]() { threadMain(); }))
{}
m_RendererType(rendererType), mName(name), mAffinity(affinity), mWorkMutex{}
{
// Don't launch the worker thread until all of our objects are fully initialized.
mThread = std::thread([this]() { threadMain(); });
}

~WorkerThread() { terminateThread(); }

Expand Down Expand Up @@ -139,15 +139,16 @@ class WorkerThread

const std::string mName;
const Affinity mAffinity;
std::thread mThread;

WorkID m_lastPushedWorkID = 0;
std::atomic<WorkID> m_lastCompletedWorkID = 0;
bool mIsTerminated = false;

std::mutex mWorkMutex;
std::queue<std::function<void(EGLThreadState*)>> mWorkQueue;
std::condition_variable_any m_workPushedCondition;
std::condition_variable_any m_workedCompletedCondition;

std::mutex mWorkMutex;
std::thread mThread;
};
} // namespace rive_android

0 comments on commit 850bc8b

Please sign in to comment.