-
Notifications
You must be signed in to change notification settings - Fork 104
autopilot manager: fix auto_restart_autopilot() race condition #3474
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 #3474
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideBackport race condition fix for autopilot manager by adding exception handling around process.cmdline() to gracefully handle Psutil NoSuchProcess errors. Class diagram for updated is_ardupilot_process logicclassDiagram
class AutopilotManager {
firmware_manager: FirmwareManager
is_ardupilot_process(process: psutil.Process) bool
}
class FirmwareManager {
firmware_path(platform: Platform) Path
}
AutopilotManager --> FirmwareManager
AutopilotManager : +is_ardupilot_process(process)
FirmwareManager : +firmware_path(platform)
%% Highlight the new exception handling
class is_ardupilot_process {
+try/except psutil.NoSuchProcess
}
File-Level Changes
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 @joaoantoniocardoso - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `core/services/ardupilot_manager/autopilot_manager.py:502` </location>
<code_context>
firmware_path = self.firmware_manager.firmware_path(platform)
- if str(firmware_path) in " ".join(process.cmdline()):
- return True
+ try:
+ if str(firmware_path) in " ".join(process.cmdline()):
+ return True
+ except psutil.NoSuchProcess:
+ # process may have died before we could call cmdline()
+ pass
return False
</code_context>
<issue_to_address>
Consider broadening exception handling to include AccessDenied and ZombieProcess.
psutil.cmdline() may also raise AccessDenied or ZombieProcess, so including these in the exception handling will improve reliability.
Suggested implementation:
```python
try:
if str(firmware_path) in " ".join(process.cmdline()):
return True
except (psutil.NoSuchProcess, psutil.AccessDenied, psutil.ZombieProcess):
# process may have died, become a zombie, or access was denied before we could call cmdline()
pass
```
If `psutil.AccessDenied` and `psutil.ZombieProcess` are not already imported in this file, ensure that `import psutil` is present at the top of the file. If only specific exceptions are imported, add these two to the import statement.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
try: | ||
if str(firmware_path) in " ".join(process.cmdline()): | ||
return True | ||
except psutil.NoSuchProcess: | ||
# process may have died before we could call cmdline() | ||
pass |
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.
suggestion: Consider broadening exception handling to include AccessDenied and ZombieProcess.
psutil.cmdline() may also raise AccessDenied or ZombieProcess, so including these in the exception handling will improve reliability.
Suggested implementation:
try:
if str(firmware_path) in " ".join(process.cmdline()):
return True
except (psutil.NoSuchProcess, psutil.AccessDenied, psutil.ZombieProcess):
# process may have died, become a zombie, or access was denied before we could call cmdline()
pass
If psutil.AccessDenied
and psutil.ZombieProcess
are not already imported in this file, ensure that import psutil
is present at the top of the file. If only specific exceptions are imported, add these two to the import statement.
This is a backport of #3401 into 1.4
Summary by Sourcery
Bug Fixes: