-
Notifications
You must be signed in to change notification settings - Fork 104
autopilot manager: fix auto_restart_autopilot() race condition #3401
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
autopilot manager: fix auto_restart_autopilot() race condition #3401
Conversation
Reviewer's GuideWrapped the process cmdline lookup in a try/except block to catch and ignore psutil.NoSuchProcess errors, preventing race conditions when inspecting terminated processes. Class diagram for updated is_ardupilot_process functionclassDiagram
class AutopilotManager {
+is_ardupilot_process(process: psutil.Process) bool
}
class psutil_Process {
+cmdline() list
}
AutopilotManager --|> psutil_Process : uses
AutopilotManager : +try/except psutil.NoSuchProcess
AutopilotManager : +ignore process if NoSuchProcess
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Williangalvani - I've reviewed your changes - here's some feedback:
- Consider logging the caught NoSuchProcess at debug level so you can monitor how often this race condition occurs.
- You could use psutil.process_iter(attrs=['cmdline']) to fetch cmdline in bulk and shrink the race window instead of calling process.cmdline() per process.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider logging the caught NoSuchProcess at debug level so you can monitor how often this race condition occurs.
- You could use psutil.process_iter(attrs=['cmdline']) to fetch cmdline in bulk and shrink the race window instead of calling process.cmdline() per process.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
if str(firmware_path) in " ".join(process.cmdline()): | ||
return True | ||
try: | ||
if str(firmware_path) in " ".join(process.cmdline()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so.. shouldn't this check for is_running
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that should work! will need re-testing though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm there would still be a chance, though. of the process stopping existing between the is_running() check and the cmdline() call.
fix #3400
needs testingtested by @vshieSummary by Sourcery
Bug Fixes:
Summary by Sourcery
Bug Fixes: