Skip to content

Conversation

@savannahostrowski
Copy link
Member

@savannahostrowski savannahostrowski commented Dec 11, 2025

This validates script/module/PID exists before starting the profiler. If the target doesn't exist, we now fail fast with a clear error instead of launching an empty TUI.

Screenshot 2025-12-11 at 11 00 21 AM Screenshot 2025-12-11 at 11 00 09 AM

@savannahostrowski
Copy link
Member Author

Hmm, there's a better fix here. Please hold :D

"""Handle live mode for an existing process."""
# Check if process exists
try:
os.kill(pid, 0)
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this works on windows. On Windows, os.kill() only supports signals SIGTERM, CTRL_C_EVENT, and CTRL_BREAK_EVENT. Signal 0 (which is the Unix idiom for checking if a process exists without sending a signal) is not supported on Windows.

Copy link
Member

Choose a reason for hiding this comment

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

This also has a TOCTOU problem: the process may die after you check and before we attach

Copy link
Member

Choose a reason for hiding this comment

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

The fix probably needs to be done more on the line of https://github.com/python/cpython/pull/142655/changes (we should handle this there)

@pablogsal
Copy link
Member

pablogsal commented Dec 14, 2025

I've made some improvements to the implementation.

  • Refactored the sync waiting logic by extracting _check_process_died() and _wait_for_ready_signal() helper functions for better readability. Replaced select.select with selectors.DefaultSelector which has a nicer interface.

  • Changed stderr to always be captured so we can show clean error messages. Added try/except around _run_with_sync() calls to convert RuntimeError to clean sys.exit() messages so non-existent scripts/modules now show clean errors without tracebacks:

  • Also fixed absolute path validation which was incorrectly prepending cwd to absolute paths.

@pablogsal pablogsal force-pushed the tachyon-file-does-not-exist branch 3 times, most recently from fe664f9 to 980b035 Compare December 14, 2025 03:16
Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks a lot for the PR @savannahostrowski ❤️

@pablogsal pablogsal merged commit f893e8f into python:main Dec 14, 2025
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants