chore(liblogosdelivery): node_api destroy move + security/resource fixes#3736
chore(liblogosdelivery): node_api destroy move + security/resource fixes#3736Ivansete-status merged 1 commit intomasterfrom
Conversation
…ible resource leak
There was a problem hiding this comment.
Pull request overview
This PR addresses security concerns in the FFI API and adds health status monitoring capabilities to liblogosdelivery. The changes improve resource management, add partial race condition protection for event callbacks, and enable connection status monitoring through the FFI interface.
Changes:
- Added Lock-based protection for event callback setter to mitigate race conditions
- Implemented resource cleanup when FFI thread request submission fails during node creation
- Added EventConnectionStatusChange listener to expose health status events to FFI consumers
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| liblogosdelivery/liblogosdelivery.nim | Removed logosdelivery_destroy function (relocated to node_api.nim) |
| liblogosdelivery/declare_lib.nim | Added Lock import and lock-protected event callback setter to prevent race conditions |
| liblogosdelivery/logos_delivery_api/node_api.nim | Added logosdelivery_destroy function, health event listener, resource cleanup on creation failure, and event listener cleanup in stop |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $newJsonEvent("connection_status_change", event), | ||
| ).valueOr: | ||
| chronicles.error "ConnectionStatusChange.listen failed", err = $error | ||
| return err("ConnectionStatusChange.listen failed: " & $error) |
There was a problem hiding this comment.
If event listener registration fails (e.g., MessageSentEvent.listen succeeds but MessageErrorEvent.listen fails), the function returns early without cleaning up the previously registered listeners. This could lead to orphaned event listeners that continue to trigger callbacks even though the node never fully started. Consider adding cleanup logic in the error paths, or using a defer block with a success flag to ensure listeners are cleaned up on failure.
| echo "error: invalid context in logosdelivery_set_event_callback" | ||
| return | ||
|
|
||
| # prevent race conditions that might happen due incorrect usage. |
There was a problem hiding this comment.
Spelling error in comment: "due incorrect usage" should be "due to incorrect usage".
| # prevent race conditions that might happen due incorrect usage. | |
| # prevent race conditions that might happen due to incorrect usage. |
| chronicles.error "MessagePropagatedEvent.listen failed", err = $error | ||
| return err("MessagePropagatedEvent.listen failed: " & $error) | ||
|
|
||
| let ConnectionStatusChangeListener = EventConnectionStatusChange.listen( |
There was a problem hiding this comment.
The variable name ConnectionStatusChangeListener should follow camelCase convention to be consistent with the other listener variables (sentListener, errorListener, propagatedListener) in the same function. Consider renaming it to connectionStatusChangeListener.
| let ConnectionStatusChangeListener = EventConnectionStatusChange.listen( | |
| let connectionStatusChangeListener = EventConnectionStatusChange.listen( |
| $newJsonEvent("connection_status_change", event), | ||
| ).valueOr: | ||
| chronicles.error "ConnectionStatusChange.listen failed", err = $error | ||
| return err("ConnectionStatusChange.listen failed: " & $error) |
There was a problem hiding this comment.
The listener variables (sentListener, errorListener, propagatedListener, ConnectionStatusChangeListener) are assigned but never used. While this may be intentional if the listeners are automatically managed by the event broker system, consider whether these variables should be stored or if they can be ignored with an underscore prefix (e.g., _ = MessageSentEvent.listen(...)). If they need to be stored for cleanup, they should be saved in the context. Note that cleanup currently uses dropAllListeners which doesn't require these handles.
| MessageErrorEvent.dropAllListeners(ctx.myLib[].brokerCtx) | ||
| MessageSentEvent.dropAllListeners(ctx.myLib[].brokerCtx) | ||
| MessagePropagatedEvent.dropAllListeners(ctx.myLib[].brokerCtx) | ||
| EventConnectionStatusChange.dropAllListeners(ctx.myLib[].brokerCtx) |
There was a problem hiding this comment.
Event listener cleanup in logosdelivery_stop_node should ideally happen after the node is stopped, not before. If the node emits events during shutdown, those events won't be properly handled since the listeners have already been dropped. Consider moving the dropAllListeners calls after the ctx.myLib[].stop() call, or handle this within the stop implementation itself.
4e74433 to
8e41a27
Compare
Summary
Scope after split