Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RSDK-9610 - reconnect loop should exit if it fails #814

Merged

Conversation

stuqdog
Copy link
Member

@stuqdog stuqdog commented Jan 7, 2025

Causes the _check_connection method to call sys.exit() if it reaches the end of the reconnect loop and remains disconnected. Without doing so, it remains alive and attempting to reconnect forever whenever RDK quits non-gracefully, causing a memory leak.

Returning is not sufficient, because the thread still exists and remains managed, so the server's await on process closing never returns. By exiting, the parent module recognizes that its processes have completed and shuts down successfully.

Tested locally by having RDK shutdown with exit code 1 abruptly. With this change, the python module exits once the reconnect loop completes. Without this change, the python module stays alive indefinitely until killed.

@stuqdog stuqdog requested a review from a team as a code owner January 7, 2025 15:50
Comment on lines +435 to +437
if not self._connected:
# We failed to reconnect, sys.exit() so that this thread doesn't stick around forever.
sys.exit()
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that it will exit if it fails to reconnect once? I remember I did some work a few years ago that had to do with supporting reconnect attempts, so wanted to make sure this was something separate.

Copy link
Member

Choose a reason for hiding this comment

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

I have since expanded the code above this. 😅 LGTM!

Comment on lines +435 to +437
if not self._connected:
# We failed to reconnect, sys.exit() so that this thread doesn't stick around forever.
sys.exit()
Copy link
Member

Choose a reason for hiding this comment

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

I have since expanded the code above this. 😅 LGTM!

@stuqdog stuqdog merged commit c69a7d7 into viamrobotics:main Jan 8, 2025
12 checks passed
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