Skip to content

feat: Lock Array and Object wrappers for safe multithreading #164

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

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

mrousavy
Copy link
Member

@mrousavy mrousavy commented Apr 8, 2024

Adds locks to array and object wrappers to make multi-threaded access (read or write operations, e.g. push() and pop()) safe.

Uses a recursive_mutex, might have a small performance impact.

@chrfalch
Copy link
Collaborator

chrfalch commented Apr 9, 2024

I finally got around to read the documentation about concurrency and containers in the C++ standard, and I think we could avoid a lot of the locking in the PR - read more here - I'll try to go through and "mark" methods that we need to be careful about a bit later:

https://en.cppreference.com/w/cpp/container#Thread_safety

@mrousavy
Copy link
Member Author

mrousavy commented Apr 9, 2024

@chrfalch which methods are you talking about? Consider the following code:

const array = useSharedValue([35, 75234, 385, 357])

const threadA = useWorklet(context1, () => {
  'worklet'
  for (let i = 0; i < array.value.length; i++) {
    console.log(array.value[i])
  }
})

const threadB = useWorklet(context2, () => {
  'worklet'
  while (array.value.length > 0) {
    const item = array.value.pop()
    console.log(item)
  }
})

threadA()
threadB()

In threadB we remove all items, and in threadA we iterate through the items with said index.

Without throwing a lock on pop and index, we would probably encounter issues here.

@mrousavy
Copy link
Member Author

mrousavy commented Apr 9, 2024

Also, check out how pop() is implemented:

JSI_HOST_FUNCTION(pop) {
// Pop last element from array
if (_array.empty()) {
return jsi::Value::undefined();
}
auto lastEl = _array.at(_array.size() - 1);
_array.pop_back();
notify();
return lastEl->unwrapAsProxyOrValue(runtime);
};

We have three separate operations on our std::vector:

  1. _array.size()
  2. _array.at(..)
  3. _array.pop_back()

And for example, after step 1 (size()), another Thread might've already popped an item in the meantime, so the step 2 (at(..)) call might already be out of bounds.

Unless I'm missing something we need to lock on that entire pop() method

@mrousavy
Copy link
Member Author

mrousavy commented Apr 9, 2024

Without the lock PR, I get this error randomly after leaving the app run for a while where arrays are accessed from multiple threads:

image

@mrousavy mrousavy merged commit 4b27e31 into main Apr 9, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants