BUG: Process not found stats #764
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR adds robust error handling to file reads in the proc-based process stats path. The common.cat helper now catches OSError and returns None, and get_process_info in processes.py checks for None on stat, loginuid, and statm reads, bailing out early with an empty result when a proc file is missing to prevent uncaught exceptions. Class diagram for updated process info retrievalclassDiagram
class Processes {
+get_process_info(pid, gpu_mem_usage, process_name, uptime)
usernames: dict
}
class Common {
+cat(path)
}
Processes --> Common : uses
Processes : +get_process_info checks for None from cat()
Common : +cat returns None on OSError
Flow diagram for robust process info retrieval with error handlingflowchart TD
A["get_process_info() called"] --> B["Check if /proc/<pid> exists"]
B -->|exists| C["Read /proc/<pid>/stat with cat()"]
C -->|cat returns None| Z["Return [] (bail out)"]
C -->|cat returns value| D["Read /proc/<pid>/loginuid with cat()"]
D -->|cat returns None| Z
D -->|cat returns value| E["Read /proc/<pid>/statm with cat()"]
E -->|cat returns None| Z
E -->|cat returns value| F["Parse and return process info"]
B -->|does not exist| Z
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The repeated None checks after each cat invocation could be consolidated into a helper or decorator to reduce boilerplate and improve readability.
- Silently returning an empty list when a proc file is missing might hide intermittent failures; consider adding a debug log when skipping a process to aid troubleshooting.
- The cat function currently catches all OSError exceptions—restricting that to FileNotFoundError (or using a custom exception) would avoid masking other file-related errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated None checks after each cat invocation could be consolidated into a helper or decorator to reduce boilerplate and improve readability.
- Silently returning an empty list when a proc file is missing might hide intermittent failures; consider adding a debug log when skipping a process to aid troubleshooting.
- The cat function currently catches all OSError exceptions—restricting that to FileNotFoundError (or using a custom exception) would avoid masking other file-related errors.
## Individual Comments
### Comment 1
<location> `jtop/core/processes.py:108` </location>
<code_context>
# VmRSS is the resident set size of the process, which is the portion of the process's memory
# that is held in RAM and is not swapped out to disk. This is the amount of memory that the process is currently using.
mem_raw = cat(os.path.join('/proc', pid, 'statm')).split()
+ if mem_raw is None:
+ return []
vm_rss = int(mem_raw[1]) * 4
</code_context>
<issue_to_address>
**issue:** Check for empty list after split to avoid IndexError.
If mem_raw is empty, accessing mem_raw[1] will cause an IndexError. Please check the list length before accessing its elements.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes the issue of reading process stats when the process is not found. Although there is a check before to check if the process exists, there are chances when the process gets killed between each check. The previous implementation takes the raw file content and parses it, disregarding
OSErrors. This PR adds security to ensure safe resolution ofOSErrorto get process info.Logs
Here is the
jtop.servicelogs.Summary by Sourcery
Handle missing or removed processes gracefully by catching file errors when reading /proc files and skipping stats for processes that vanish between checks
Bug Fixes: