Skip to content

fix: gracefully handle disappearing procs#27

Merged
ericaltendorf merged 4 commits into
mainfrom
race-condition-fix
Feb 12, 2021
Merged

fix: gracefully handle disappearing procs#27
ericaltendorf merged 4 commits into
mainfrom
race-condition-fix

Conversation

@ericaltendorf

Copy link
Copy Markdown
Owner

and fix inconsistencies between versions of job-getter funcs by combining them

…tween versions of job-getter funcs by combining them

@altendky altendky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Note that I'm not checking completeness of the refactor. There may be leftover references to the removed function. Normally tests with coverage checks would account for that (mostly).

Approving despite suggestions since the suggestions aren't obviously significantly better. But maybe still of interest.

Comment thread job.py Outdated
Comment thread job.py Outdated
ericaltendorf and others added 2 commits February 11, 2021 20:36
Co-authored-by: Kyle Altendorf <sda@fstab.net>
Co-authored-by: Kyle Altendorf <sda@fstab.net>

@altendky altendky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorry, the benefit of using an empty iterable as the default was to eliminate the special case handling code of when it was None. Just makes it a tad easier to read. Especially the second if where the and short-circuit was being leveraged to avoid accessing a not-defined variable. (where I think both the maybe-defined variable and the short-circuit dependency are a bit smelly individually)

Comment thread job.py Outdated
Comment on lines +63 to +64
if cached_jobs:
cached_jobs_by_pid = { j.proc.pid: j for j in cached_jobs }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if cached_jobs:
cached_jobs_by_pid = { j.proc.pid: j for j in cached_jobs }
cached_jobs_by_pid = { j.proc.pid: j for j in cached_jobs }

Comment thread job.py Outdated
# iteration and data access.
with contextlib.suppress(psutil.NoSuchProcess):
if is_plotting_cmdline(proc.cmdline()):
if cached_jobs and proc.pid in cached_jobs_by_pid.keys():

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if cached_jobs and proc.pid in cached_jobs_by_pid.keys():
if proc.pid in cached_jobs_by_pid.keys():

@ericaltendorf ericaltendorf merged commit 31ccb8f into main Feb 12, 2021
@ericaltendorf ericaltendorf deleted the race-condition-fix branch February 12, 2021 05:02
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.

2 participants