Add callback for first event registered, and last unregistered #175
Conversation
(cherry picked from commit 5ca4244)
|
@coderabbitai review pls |
|
@Segfaultd: I'll review the changes in this pull request for you. ✅ Actions performedReview triggered.
|
WalkthroughThe changes add event listener removal functionality to the scripting framework. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Lua
participant Event
participant Engine
Lua->>Event: on(name, fnc)
Event->>Engine: ListenEvent(name, fnc)
activate Engine
Engine->>Engine: Check if first listener
alt First listener
Engine->>Engine: Trigger _onFirstEventRegisteredProc
end
Engine->>Engine: Add listener to map
deactivate Engine
Lua->>Event: remove(name, fnc)
Event->>Engine: RemoveEventListener(name, fnc)
activate Engine
Engine->>Engine: Remove listener from map
Engine->>Engine: Check if last listener
alt Last listener removed
Engine->>Engine: Trigger _onLastEventUnregisteredProc
Engine->>Engine: Remove event key
end
deactivate Engine
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/framework/src/scripting/builtins/event.h(2 hunks)code/framework/src/scripting/engine.h(4 hunks)
🔇 Additional comments (7)
code/framework/src/scripting/builtins/event.h (2)
13-15: LGTM! Clean implementation.The
Removemethod follows the same pattern as the existingOnmethod and appropriately delegates to the engine.
25-25: LGTM! Proper Lua exposure.The binding provides a clean API for removing event listeners from Lua scripts.
code/framework/src/scripting/engine.h (5)
26-26: LGTM! Clean type alias.The callback signature is appropriate for event lifecycle notifications.
40-41: LGTM! Consistent with existing callback pattern.The callback members follow the same initialization pattern as other callbacks in the class.
153-159: LGTM! Consistent setter implementation.The setter methods follow the established pattern in the class and properly use
std::movefor efficiency.
86-88: Consider lifecycle callbacks for remote events.The new lifecycle callbacks only apply to local events (
_eventHandlers), but not to remote events (_eventRemoteHandlers). While this may be intentional based on the use case, it's worth confirming whether remote events should also support first-registration/last-unregistration callbacks for consistency.Should
ListenRemoteEventalso track first registration and last unregistration? If so, aRemoveRemoteEventListenermethod would also be needed.
72-76: No changes required—sol::functioncomparison works as intended.The
sol::function::operator==uses the reference comparator by calling Lua's equality (lua_comparewithLUA_OPEQ) on the two referenced values, following Lua's equality semantics (function identity in Lua). This means the code correctly removes the listener by identity, as intended.
| void ListenEvent(const std::string &name, sol::function fnc) { | ||
| auto &handlers = _eventHandlers[name]; | ||
| bool firstRegistration = handlers.empty(); | ||
|
|
||
| handlers.push_back(std::move(fnc)); | ||
|
|
||
| if (firstRegistration && _onFirstEventRegisteredProc) { | ||
| _onFirstEventRegisteredProc(name); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Data race on _eventHandlers.
ListenEvent modifies _eventHandlers without holding _executionMutex, but InvokeEvent (line 94) locks the mutex before iterating over the same map. This creates a data race where concurrent listener registration and event invocation can corrupt the map or cause undefined behavior.
Apply this diff to add mutex protection:
void ListenEvent(const std::string &name, sol::function fnc) {
+ std::lock_guard<std::mutex> lock(_executionMutex);
+
auto &handlers = _eventHandlers[name];
bool firstRegistration = handlers.empty();
handlers.push_back(std::move(fnc));
if (firstRegistration && _onFirstEventRegisteredProc) {
_onFirstEventRegisteredProc(name);
}
}Note: If the callback _onFirstEventRegisteredProc might call back into the engine or take locks, consider invoking it after releasing the mutex to avoid potential deadlocks.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void ListenEvent(const std::string &name, sol::function fnc) { | |
| auto &handlers = _eventHandlers[name]; | |
| bool firstRegistration = handlers.empty(); | |
| handlers.push_back(std::move(fnc)); | |
| if (firstRegistration && _onFirstEventRegisteredProc) { | |
| _onFirstEventRegisteredProc(name); | |
| } | |
| } | |
| void ListenEvent(const std::string &name, sol::function fnc) { | |
| std::lock_guard<std::mutex> lock(_executionMutex); | |
| auto &handlers = _eventHandlers[name]; | |
| bool firstRegistration = handlers.empty(); | |
| handlers.push_back(std::move(fnc)); | |
| if (firstRegistration && _onFirstEventRegisteredProc) { | |
| _onFirstEventRegisteredProc(name); | |
| } | |
| } |
🤖 Prompt for AI Agents
In code/framework/src/scripting/engine.h around lines 54 to 63, ListenEvent
mutates the shared _eventHandlers map without acquiring _executionMutex while
InvokeEvent locks that same mutex, causing a data race; fix by acquiring the
mutex (e.g., std::scoped_lock or std::lock_guard on _executionMutex) around the
access that reads/creates the handlers and pushes the callback, record whether
this was the first registration while still holding the lock, then release the
lock before calling _onFirstEventRegisteredProc(name) (to avoid potential
deadlocks if that callback re-enters the engine); ensure the handler is
moved/added while locked and only invoke external callbacks after unlocking.
| void RemoveEventListener(const std::string &name, const sol::function &fnc) { | ||
| auto it = _eventHandlers.find(name); | ||
| if (it == _eventHandlers.end()) | ||
| return; | ||
|
|
||
| auto &handlers = it->second; | ||
|
|
||
| handlers.erase(std::remove_if(handlers.begin(), handlers.end(), | ||
| [&](const sol::function &f) { | ||
| return f == fnc; | ||
| }), | ||
| handlers.end()); | ||
|
|
||
| if (handlers.empty()) { | ||
| _eventHandlers.erase(it); | ||
| if (_onLastEventUnregisteredProc) { | ||
| _onLastEventUnregisteredProc(name); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Data race on _eventHandlers.
RemoveEventListener modifies _eventHandlers without mutex protection, creating the same data race issue as ListenEvent. Concurrent removal and event invocation can lead to undefined behavior.
Apply this diff to add mutex protection:
void RemoveEventListener(const std::string &name, const sol::function &fnc) {
+ std::lock_guard<std::mutex> lock(_executionMutex);
+
auto it = _eventHandlers.find(name);
if (it == _eventHandlers.end())
return;
auto &handlers = it->second;
handlers.erase(std::remove_if(handlers.begin(), handlers.end(),
[&](const sol::function &f) {
return f == fnc;
}),
handlers.end());
if (handlers.empty()) {
_eventHandlers.erase(it);
if (_onLastEventUnregisteredProc) {
_onLastEventUnregisteredProc(name);
}
}
}Note: Similar to ListenEvent, consider whether _onLastEventUnregisteredProc should be invoked outside the lock if it might interact with the engine.
🤖 Prompt for AI Agents
In code/framework/src/scripting/engine.h around lines 65 to 84,
RemoveEventListener mutates _eventHandlers without synchronization causing a
data race; protect the function with the same mutex used by ListenEvent (acquire
a std::lock_guard or equivalent at the start of the function) while finding,
modifying and potentially erasing the handlers entry, and only call
_onLastEventUnregisteredProc after releasing the lock (or document/decide if it
must be invoked inside the lock) to avoid deadlocks or reentrancy issues.
3 participants
Motivation: You may have "spammy events" and you don't want to enable system that trigger them when user don't want it to save performance, for example position changed events, collision events
Summary by CodeRabbit
Release Notes