fixed thread safe inject#61
Conversation
|
Warning Review limit reached
More reviews will be available in 15 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds thread-safety to the IoC service container by introducing mutex-based synchronization. The change adds a 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
||
| ~ThreadSafeInjectBase() | ||
| { | ||
| std::shared_lock lock(m_mutex); |
There was a problem hiding this comment.
I think it should be unique_lock, because this is a mutation
|
|
||
| private: | ||
| mutable std::shared_mutex m_mutex; | ||
| static inline std::shared_mutex m_mutex; |
There was a problem hiding this comment.
This looks incorrect to me; the mutex is supposed to guard access to members of this ThreadSafeInjectBase instance only, when the same instance is used by multiple threads. Not to guard interaction between multiple instances, because multiple instances don't (shouldn't) interact at all. Therefore, making this mutex static causes multiple instances to unnecessarily wait for each other, and it makes the intention of this mutex unclear.
There was a problem hiding this comment.
Now this mutex protects not only "its" class member, but also the IoC itself, or more precisely, the modifications to the IoC that occur during subscribe and unsubscribe.
I thought about what options there are:
- Adding a mutex to the IOC itself is a typical solution. But it will always be called, and in our case, 95% of the calls are in a single thread. But maybe we're making a micro-optimization.
- Make one static mutex for all.
While writing these options, I realized they don't fully solve the problem. We're left with only option 1.
The only thing we can do is make sure the mutex is locked once, not twice: protecting the Inject class member and the subscriber list.
There was a problem hiding this comment.
Or, alternatively, we can disable the subscription for these thread injectors. It was needed primarily to break cyclic dependencies when deleting the IOC. I think these can be sacrificed here and controlled manually.
| m_i = ioc->template resolve<I>(module); | ||
| if (m_i) { | ||
| m_subscriber.onChanged = [this](const std::shared_ptr<I>& p) { | ||
| m_i = p; |
There was a problem hiding this comment.
Claude flagged a possible data race here, inside the callback, where m_i is modified without std::unique_lock
igorkorsukov
force-pushed
the
w/ioc/threadsafe
branch
from
7f700fb to
013a64c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/global/thirdparty/kors_modularity/modularity/ioc.h`:
- Line 132: The subscription flag m_isSubscribe in InjectBase<I> is non-mutable
but is modified inside the const method ThreadSafeInjectBase<I>::get(), so make
m_isSubscribe mutable to preserve const-correctness: change the declaration of
InjectBase<I>::m_isSubscribe to be mutable (e.g., mutable bool m_isSubscribe =
true) so ThreadSafeInjectBase<I>::get() const can assign to it without breaking
const rules; update any comments if necessary to document why it is mutable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5d5a7f3-351c-43e4-8895-09c2f656337a
📒 Files selected for processing (1)
framework/global/thirdparty/kors_modularity/modularity/ioc.h
igorkorsukov
force-pushed
the
w/ioc/threadsafe
branch
from
013a64c to
df53205
Compare
|
@cbjeukendrup What do you think about the new solution with the |
|
From the technical perspective, I see no problems with it. I don't have a strong opinion about the behaviour; one could argue that it's unexpected that one version of Inject does a thing that another version (which is meant as a drop-in replacement for different circumstances) does not, but it's not a big deal. I must say I had not entirely followed the introduction of this subscription concept, so I'm not sure exactly what it does and how important it is. |
|
The initial problem was that we had cyclic dependencies, and since a shared_ptr was used everywhere, this led to services not being deleted when the IoC was destroyed, when the window was closed. This became important in single-process mode. This subscription system solves this problem. When a service is deleted, a notification is sent about its change, and everyone who used it clear their links. |
|
That makes sense. That does sound like something that is equally desirable for the thread-safe Inject... removing it from there is okay as a temporary solution but does feel a bit like tech debt. Just had another idea: what if we get rid of ThreadSafeInject, and use a local How good that solution is, depends a little bit on how often such a background thread method is called, because of course this way the dependency is resolved again and again on every call. Hm... actually that still doesn't solve the problem that |
|
Thinking about it more, just adding a mutex to ModulesIoCBase is perhaps not as bad as it seems. It would only be used when a new |
igorkorsukov
force-pushed
the
w/ioc/threadsafe
branch
from
df53205 to
362ad33
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
framework/global/thirdparty/kors_modularity/modularity/modulesioc.h (3)
140-152:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: Deadlock risk from invoking callbacks while holding the mutex.
The
reset()method holdsm_mutexwhile iterating overinj.onChangesand invoking user callbacks (line 148). These callbacks can re-enter the IoC container by callingresolve(),unsubscribe(), or other methods—all of which now attempt to acquire the same non-recursivem_mutex, causing a deadlock.Additionally, if a callback destroys a
Subscriber, its destructor callsunsubscribe()→onUnSubscribe()(the lambda frommakeUnSubscribeFn) →doUnsubscribe()→ attempts to lockm_mutex→ deadlock.Even switching to
std::recursive_mutexwould not fully solve this, because if a callback modifiesinj.onChangesduring iteration, you'd have iterator invalidation and undefined behavior.🔒 Proposed fix: invoke callbacks without holding the lock
void reset() { + std::vector<std::pair<std::string, std::vector<OnChangedInternal>>> callbacks_to_invoke; + + { - std::lock_guard<std::mutex> lock(m_mutex); + std::lock_guard<std::mutex> lock(m_mutex); - for (auto& s : m_map) { - Service& inj = s.second; - inj.p = nullptr; - - for (const auto& c : inj.onChanges) { - c.second(inj.p); + for (auto& s : m_map) { + Service& inj = s.second; + inj.p = nullptr; + + if (!inj.onChanges.empty()) { + std::vector<OnChangedInternal> cbs; + cbs.reserve(inj.onChanges.size()); + for (const auto& c : inj.onChanges) { + cbs.push_back(c.second); + } + callbacks_to_invoke.emplace_back(s.first, std::move(cbs)); + } } + m_map.clear(); } - m_map.clear(); + + // Invoke callbacks without holding the lock + for (const auto& pair : callbacks_to_invoke) { + for (const auto& cb : pair.second) { + cb(nullptr); + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/global/thirdparty/kors_modularity/modularity/modulesioc.h` around lines 140 - 152, reset() currently invokes Service::onChanges callbacks while holding m_mutex, risking deadlock and iterator invalidation; fix by acquiring m_mutex, for each entry in m_map set Service::p = nullptr and copy or move the onChanges callbacks for that service into a local container, then clear m_map and release the lock, and only after unlocking iterate the collected callbacks and invoke them (passing the nullptr) so callbacks run without holding m_mutex; reference symbols: reset(), m_mutex, m_map, Service, onChanges, Service::p.
158-172:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: Deadlock risk from invoking callbacks while holding the mutex.
The
unregisterService()method invokes user callbacks (line 170) while holdingm_mutex. This has the same deadlock risk asreset(): if any callback calls back into the IoC container, it will attempt to acquire the already-held non-recursive mutex and deadlock.🔒 Proposed fix: invoke callbacks without holding the lock
void unregisterService(const InterfaceInfo& info) { + std::vector<OnChangedInternal> callbacks_to_invoke; + + { - std::lock_guard<std::mutex> lock(m_mutex); + std::lock_guard<std::mutex> lock(m_mutex); - auto it = m_map.find(info.id); - if (it == m_map.end()) { - return; - } - - Service& inj = it->second; - inj.p = nullptr; - - for (const auto& c : inj.onChanges) { - c.second(inj.p); + auto it = m_map.find(info.id); + if (it == m_map.end()) { + return; + } + + Service& inj = it->second; + inj.p = nullptr; + + callbacks_to_invoke.reserve(inj.onChanges.size()); + for (const auto& c : inj.onChanges) { + callbacks_to_invoke.push_back(c.second); + } } + + // Invoke callbacks without holding the lock + for (const auto& cb : callbacks_to_invoke) { + cb(nullptr); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/global/thirdparty/kors_modularity/modularity/modulesioc.h` around lines 158 - 172, unregisterService currently holds m_mutex while invoking user callbacks (onChanges) which can deadlock if callbacks re-enter the IoC; to fix, capture the Service state you need (e.g., copy inj.p and the list of callbacks from it) while holding the lock, then release the lock before invoking any callbacks—specifically, inside unregisterService lock m_mutex, find the Service in m_map, set inj.p = nullptr, copy inj.onChanges (or the callbacks values) into a local container, then let the lock_guard go out of scope and only then iterate the copied callbacks and call them with the copied inj.p; ensure you reference unregisterService, Service, m_map, m_mutex, onChanges and inj.p when making the change.
174-204:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: Deadlock risk from invoking callbacks while holding the mutex.
The
registerService()method invokes user callbacks (line 195) while holdingm_mutexwhen re-registering a previously unregistered service. This has the same deadlock risk: callbacks that call back into the IoC will attempt to acquire the already-held non-recursive mutex.🔒 Proposed fix: invoke callbacks without holding the lock
void registerService(const std::string& module, const InterfaceInfo& info, std::shared_ptr<IModuleInterface> p) { if (!p) { assert(p); return; } + std::vector<OnChangedInternal> callbacks_to_invoke; + + { - std::lock_guard<std::mutex> lock(m_mutex); + std::lock_guard<std::mutex> lock(m_mutex); - auto foundIt = m_map.find(info.id); - if (foundIt != m_map.end()) { - Service& inj = foundIt->second; - if (inj.p) { - std::cerr << module << ": double register:" - << info.id << ", first register in" << m_map[info.id].sourceModule << std::endl; - assert(false); - } else { - inj.sourceModule = module; - inj.p = p; - for (const auto& c : inj.onChanges) { - c.second(inj.p); + auto foundIt = m_map.find(info.id); + if (foundIt != m_map.end()) { + Service& inj = foundIt->second; + if (inj.p) { + std::cerr << module << ": double register:" + << info.id << ", first register in" << m_map[info.id].sourceModule << std::endl; + assert(false); + } else { + inj.sourceModule = module; + inj.p = p; + + callbacks_to_invoke.reserve(inj.onChanges.size()); + for (const auto& c : inj.onChanges) { + callbacks_to_invoke.push_back(c.second); + } } + } else { + Service inj; + inj.sourceModule = module; + inj.p = p; + m_map[info.id] = inj; } - } else { - Service inj; - inj.sourceModule = module; - inj.p = p; - m_map[info.id] = inj; } + + // Invoke callbacks without holding the lock + // Pass the shared_ptr p, not nullptr + for (const auto& cb : callbacks_to_invoke) { + cb(p); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/global/thirdparty/kors_modularity/modularity/modulesioc.h` around lines 174 - 204, registerService currently invokes stored callbacks (inj.onChanges / Service.onChanges) while holding m_mutex, risking deadlock; fix by, when re-registering (foundIt != m_map.end() && !inj.p), while holding the lock update inj.sourceModule and inj.p and copy the callbacks (e.g., into a local std::vector of the callback functors taken from foundIt->second.onChanges), then let the lock_guard go out of scope before iterating and invoking those callbacks with inj.p; ensure you reference registerService, m_mutex, m_map[info.id] (or foundIt->second), Service.onChanges and IModuleInterface when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@framework/global/thirdparty/kors_modularity/modularity/modulesioc.h`:
- Around line 140-152: reset() currently invokes Service::onChanges callbacks
while holding m_mutex, risking deadlock and iterator invalidation; fix by
acquiring m_mutex, for each entry in m_map set Service::p = nullptr and copy or
move the onChanges callbacks for that service into a local container, then clear
m_map and release the lock, and only after unlocking iterate the collected
callbacks and invoke them (passing the nullptr) so callbacks run without holding
m_mutex; reference symbols: reset(), m_mutex, m_map, Service, onChanges,
Service::p.
- Around line 158-172: unregisterService currently holds m_mutex while invoking
user callbacks (onChanges) which can deadlock if callbacks re-enter the IoC; to
fix, capture the Service state you need (e.g., copy inj.p and the list of
callbacks from it) while holding the lock, then release the lock before invoking
any callbacks—specifically, inside unregisterService lock m_mutex, find the
Service in m_map, set inj.p = nullptr, copy inj.onChanges (or the callbacks
values) into a local container, then let the lock_guard go out of scope and only
then iterate the copied callbacks and call them with the copied inj.p; ensure
you reference unregisterService, Service, m_map, m_mutex, onChanges and inj.p
when making the change.
- Around line 174-204: registerService currently invokes stored callbacks
(inj.onChanges / Service.onChanges) while holding m_mutex, risking deadlock; fix
by, when re-registering (foundIt != m_map.end() && !inj.p), while holding the
lock update inj.sourceModule and inj.p and copy the callbacks (e.g., into a
local std::vector of the callback functors taken from
foundIt->second.onChanges), then let the lock_guard go out of scope before
iterating and invoking those callbacks with inj.p; ensure you reference
registerService, m_mutex, m_map[info.id] (or foundIt->second), Service.onChanges
and IModuleInterface when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 42c52259-f2dc-424b-97bb-b39095079656
📒 Files selected for processing (1)
framework/global/thirdparty/kors_modularity/modularity/modulesioc.h
igorkorsukov
force-pushed
the
w/ioc/threadsafe
branch
from
362ad33 to
678c301
Compare
2 participants
No description provided.