fix(💣): fix race condition in RuntimeAwareCache #3478
Conversation
Increased timeout duration for WebSocket closure.
| if (listenersSet != listeners.end()) { | ||
| for (auto listener : listenersSet->second) { | ||
| listener->onRuntimeDestroyed(_rt); | ||
| std::unordered_set<RuntimeLifecycleListener *> listenersCopy; |
There was a problem hiding this comment.
you shouldn't need to use the mutex in destructor. The destructor only operates on the listeners map that it owns. If the destructor runs concurrently while some other thread is executing a method on the RuntimeMonitorObject the problem lies somewhere else. The destructor should never run when the object is in use.
There isn't really any harm in keeping the locking code here, but in this case this may obscure more serious issues in the code that manages the object.
I understand the crash from https://gist.github.com/wcandillon/041931402613e2be876a9c1e854d4c49 is only due to concurrent access between add and removeListener methods so this change in the destructor isn't needed, right?
If you want to keep the locking here you could just move all the elements from listeners to listenersCopy using a one-liner:
listenersCopy = std::move(listeners);There was a problem hiding this comment.
The destructor modifies listeners directly, what if another runtime is added/removed concurrently to this map?
There was a problem hiding this comment.
You're right. Sorry, missed the fact this was a static field. Please discard that commend
|
Btw: not sure which version of RN you're targeting but the whole runtime lifecycle monitor could be replaced by a new mechanism added to JSI for RN 0.81: facebook/react-native@891ee78 |
|
@kmagiera thks for pointing me to |
|
🎉 This PR is included in version 2.3.7 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
2 participants
The goal of this PR is to fix a race condition where the RuntimeLifeCycle host object destructor would modify the RuntimeAwareCache concurrently (from the Hades thread). cc @kmagiera @Corelli-18512