Skip to content

add signal handling to libci - #14019

Merged
Nir-Az merged 2 commits into
realsenseai:developmentfrom
AviaAv:libci-signals
May 21, 2025
Merged

add signal handling to libci#14019
Nir-Az merged 2 commits into
realsenseai:developmentfrom
AviaAv:libci-signals

Conversation

@AviaAv

@AviaAv AviaAv commented May 21, 2025

Copy link
Copy Markdown
Contributor

Tracked on: [LRS-1260]

@AviaAv
AviaAv requested a review from Nir-Az May 21, 2025 08:42
@Nir-Az
Nir-Az requested a review from Copilot May 21, 2025 13:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds centralized signal handling to ensure proper hub disconnection on interrupts and refactors existing cleanup logic.

  • Introduces register_signal_handlers for SIGINT/SIGTERM cleanup callbacks
  • Extracts hub disconnection into close_hubs() and hooks it into signal handling
  • Automatically re-registers handlers after hub methods via __getattribute__ and in port operations

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
unit-tests/run-unit-tests.py Imported register_signal_handlers, factored out hub cleanup to close_hubs(), and wired up handlers
unit-tests/py/rspy/signals.py New module defining register_signal_handlers and default abort handler
unit-tests/py/rspy/device_hub.py Overrides __getattribute__ to wrap hub methods, re-registering signal handlers
unit-tests/py/rspy/acroname.py Added re-registration calls in enable_ports and disable_ports after sleeps
Comments suppressed due to low confidence (2)

unit-tests/run-unit-tests.py:520

  • There are no unit tests verifying that register_signal_handlers correctly invokes close_hubs on SIGINT/SIGTERM. Add tests that simulate these signals and confirm resource cleanup.
register_signal_handlers(close_hubs)

unit-tests/run-unit-tests.py:499

  • The function close_hubs references devices but there’s no import in this scope, leading to a NameError at runtime. Consider importing devices at the top or within close_hubs.
def close_hubs():

Comment on lines +15 to +28
def __getattribute__(self, name):
attr = super().__getattribute__(name)

# some hubs override / clear signals, this is used to re-register them, only for methods for now
if callable(attr) and not name.startswith('__'):
def wrapper(*args, **kwargs):
result = attr(*args, **kwargs)
signals.register_signal_handlers()
return result

return wrapper

return attr # Return non-methods or special methods as-is

Copilot AI May 21, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Overriding __getattribute__ to wrap all method calls can introduce performance overhead and reduce readability. Consider explicitly invoking register_signal_handlers in critical methods instead of a blanket override.

Suggested change
def __getattribute__(self, name):
attr = super().__getattribute__(name)
# some hubs override / clear signals, this is used to re-register them, only for methods for now
if callable(attr) and not name.startswith('__'):
def wrapper(*args, **kwargs):
result = attr(*args, **kwargs)
signals.register_signal_handlers()
return result
return wrapper
return attr # Return non-methods or special methods as-is
# Removed the __getattribute__ method as it introduces performance overhead and reduces readability.

Copilot uses AI. Check for mistakes.
changed = True
#
if changed and sleep_on_change:
signals.register_signal_handlers()

Copilot AI May 21, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Calling register_signal_handlers on every port enable/disable may incur unnecessary overhead. Consider moving handler registration to initialization rather than inside each method.

Suggested change
signals.register_signal_handlers()

Copilot uses AI. Check for mistakes.
@Nir-Az
Nir-Az merged commit 9b9334d into realsenseai:development May 21, 2025
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.

3 participants