Skip to content

gh-142654: show the clear error message when sampling on an unknown PID #142655

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.

Sign up for GitHub

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

Open
kemingy wants to merge 7 commits into
base: main
Choose a base branch
from
+56 −35
Open

gh-142654: show the clear error message when sampling on an unknown PID #142655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7024bd1
gh-142654: show the clear error message when sampling on an unknown PID
kemingy Dec 13, 2025
b1a5082
only catch the RuntimeError, leave the PermissionError for downstream
kemingy Dec 13, 2025
cfea35b
catch the PermissionError for macOS
kemingy Dec 13, 2025
ab9c0c6
check if the pid is running before attaching
kemingy Dec 15, 2025
a1c66e5
resolve conflict
kemingy Dec 15, 2025
0a1f140
Merge branch 'main' into gh-142654
kemingy Dec 15, 2025
dcad05a
fix sampling test failure due to the _is_process_running check
kemingy Dec 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Lib/profiling/sampling/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import time
from contextlib import nullcontext

from .sample import sample, sample_live
from .sample import sample, sample_live, _is_process_running
from .pstats_collector import PstatsCollector
from .stack_collector import CollapsedStackCollector, FlamegraphCollector
from .heatmap_collector import HeatmapCollector
Expand Down Expand Up @@ -743,6 +743,8 @@ def main():

def _handle_attach(args):
"""Handle the 'attach' command."""
if not _is_process_running(args.pid):
raise sys.exit(f"Process with PID {args.pid} is not running.")
Copy link
Member

@pablogsal pablogsal Dec 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would prefer to raise some custom exception here and catch it in __main__ like the other exceptions. Perhaps we should also do the same for the other usages of sys.exit.

Do you mind creating a errors.py file with custom exceptions for the different case, raise them and properly handle the messages in __main__?

Copy link
Member

@pablogsal pablogsal Dec 15, 2025

Choose a reason for hiding this comment

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

Also just FYI sys.exit already raises, using raise before it is wrong

# Check if live mode is requested
if args.live:
_handle_live_attach(args, args.pid)
Expand Down
66 changes: 36 additions & 30 deletions Lib/profiling/sampling/sample.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,22 @@ def __init__(self, pid, sample_interval_usec, all_threads, *, mode=PROFILING_MOD
self.all_threads = all_threads
self.mode = mode # Store mode for later use
self.collect_stats = collect_stats
if _FREE_THREADED_BUILD:
self.unwinder = _remote_debugging.RemoteUnwinder(
self.pid, all_threads=self.all_threads, mode=mode, native=native, gc=gc,
opcodes=opcodes, skip_non_matching_threads=skip_non_matching_threads,
cache_frames=True, stats=collect_stats
)
else:
only_active_threads = bool(self.all_threads)
self.unwinder = _remote_debugging.RemoteUnwinder(
self.pid, only_active_thread=only_active_threads, mode=mode, native=native, gc=gc,
opcodes=opcodes, skip_non_matching_threads=skip_non_matching_threads,
cache_frames=True, stats=collect_stats
)
try:
Copy link
Member

@pablogsal pablogsal Dec 15, 2025

Choose a reason for hiding this comment

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

I would normally advise against gigantic try/except blocks. can you factor the body into its own function?

if _FREE_THREADED_BUILD:
self.unwinder = _remote_debugging.RemoteUnwinder(
self.pid, all_threads=self.all_threads, mode=mode, native=native, gc=gc,
opcodes=opcodes, skip_non_matching_threads=skip_non_matching_threads,
cache_frames=True, stats=collect_stats
)
else:
only_active_threads = bool(self.all_threads)
self.unwinder = _remote_debugging.RemoteUnwinder(
self.pid, only_active_thread=only_active_threads, mode=mode, native=native, gc=gc,
opcodes=opcodes, skip_non_matching_threads=skip_non_matching_threads,
cache_frames=True, stats=collect_stats
)
except RuntimeError as err:
raise SystemExit(err)
# Track sample intervals and total sample count
self.sample_intervals = deque(maxlen=100)
self.total_samples = 0
Expand Down Expand Up @@ -86,7 +89,7 @@ def sample(self, collector, duration_sec=10, *, async_aware=False):
collector.collect_failed_sample()
errors += 1
except Exception as e:
if not self._is_process_running():
if not _is_process_running(self.pid):
break
raise e from None

Expand Down Expand Up @@ -148,22 +151,6 @@ def sample(self, collector, duration_sec=10, *, async_aware=False):
f"({(expected_samples - num_samples) / expected_samples * 100:.2f}%)"
)

def _is_process_running(self):
if sys.platform == "linux" or sys.platform == "darwin":
try:
os.kill(self.pid, 0)
return True
except ProcessLookupError:
return False
elif sys.platform == "win32":
try:
_remote_debugging.RemoteUnwinder(self.pid)
except Exception:
return False
return True
else:
raise ValueError(f"Unsupported platform: {sys.platform}")

def _print_realtime_stats(self):
"""Print real-time sampling statistics."""
if len(self.sample_intervals) < 2:
Expand Down Expand Up @@ -279,6 +266,25 @@ def _print_unwinder_stats(self):
print(f" {ANSIColors.YELLOW}Stale cache invalidations: {stale_invalidations}{ANSIColors.RESET}")


def _is_process_running(pid):
Copy link
Member

@pablogsal pablogsal Dec 15, 2025

Choose a reason for hiding this comment

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

Why was this just moved over here?

if pid <= 0:
return False
if sys.platform == "linux" or sys.platform == "darwin":
try:
os.kill(pid, 0)
return True
except ProcessLookupError:
return False
elif sys.platform == "win32":
try:
_remote_debugging.RemoteUnwinder(pid)
except Exception:
return False
return True
else:
raise ValueError(f"Unsupported platform: {sys.platform}")


def sample(
pid,
collector,
Expand Down
7 changes: 7 additions & 0 deletions Lib/test/test_profiling/test_sampling_profiler/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,7 @@ def test_cli_default_collapsed_filename(self):

with (
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli._is_process_running", return_value=True),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
main()
Expand Down Expand Up @@ -475,6 +476,7 @@ def test_cli_custom_output_filenames(self):
for test_args, expected_filename, expected_format in test_cases:
with (
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli._is_process_running", return_value=True),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
main()
Expand Down Expand Up @@ -513,6 +515,7 @@ def test_argument_parsing_basic(self):

with (
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli._is_process_running", return_value=True),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
main()
Expand All @@ -534,6 +537,7 @@ def test_sort_options(self):

with (
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli._is_process_running", return_value=True),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
main()
Expand All @@ -547,6 +551,7 @@ def test_async_aware_flag_defaults_to_running(self):

with (
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli._is_process_running", return_value=True),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
main()
Expand All @@ -562,6 +567,7 @@ def test_async_aware_with_async_mode_all(self):

with (
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli._is_process_running", return_value=True),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
main()
Expand All @@ -576,6 +582,7 @@ def test_async_aware_default_is_none(self):

with (
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli._is_process_running", return_value=True),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
main()
Expand Down
8 changes: 4 additions & 4 deletions Lib/test/test_profiling/test_sampling_profiler/test_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import profiling.sampling.sample
from profiling.sampling.pstats_collector import PstatsCollector
from profiling.sampling.stack_collector import CollapsedStackCollector
from profiling.sampling.sample import SampleProfiler
from profiling.sampling.sample import SampleProfiler, _is_process_running
except ImportError:
raise unittest.SkipTest(
"Test only runs when _remote_debugging is available"
Expand Down Expand Up @@ -602,7 +602,7 @@ def test_sample_target_module(self):
@requires_remote_subprocess_debugging()
class TestSampleProfilerErrorHandling(unittest.TestCase):
def test_invalid_pid(self):
with self.assertRaises((OSError, RuntimeError)):
with self.assertRaises((SystemExit, PermissionError)):
collector = PstatsCollector(sample_interval_usec=100, skip_idle=False)
profiling.sampling.sample.sample(-1, collector, duration_sec=1)

Expand Down Expand Up @@ -638,7 +638,7 @@ def test_is_process_running(self):
sample_interval_usec=1000,
all_threads=False,
)
self.assertTrue(profiler._is_process_running())
self.assertTrue(_is_process_running(profiler.pid))
self.assertIsNotNone(profiler.unwinder.get_stack_trace())
subproc.process.kill()
subproc.process.wait()
Expand All @@ -647,7 +647,7 @@ def test_is_process_running(self):
)

# Exit the context manager to ensure the process is terminated
self.assertFalse(profiler._is_process_running())
self.assertFalse(_is_process_running(profiler.pid))
self.assertRaises(
ProcessLookupError, profiler.unwinder.get_stack_trace
)
Expand Down
4 changes: 4 additions & 0 deletions Lib/test/test_profiling/test_sampling_profiler/test_modes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ def test_gil_mode_validation(self):

with (
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli._is_process_running", return_value=True),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
try:
Expand Down Expand Up @@ -313,6 +314,7 @@ def test_gil_mode_cli_argument_parsing(self):

with (
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli._is_process_running", return_value=True),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
try:
Expand Down Expand Up @@ -432,6 +434,7 @@ def test_exception_mode_validation(self):

with (
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli._is_process_running", return_value=True),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
try:
Expand Down Expand Up @@ -493,6 +496,7 @@ def test_exception_mode_cli_argument_parsing(self):

with (
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli._is_process_running", return_value=True),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
try:
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2025-12-13-10-34-59.gh-issue-142654.fmm974.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Show the clearer error message when using ``profiling.sampling`` on an
unknown PID.
Loading