Skip to content
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

Ensure all Python multiprocessing tests have timeouts #300

Open
wants to merge 4 commits into
base: branch-0.41
Choose a base branch
from

Conversation

pentschev
Copy link
Member

Ensure all Python multiprocessing tests timeout during join and get terminated properly, appropriately raising an error if the subprocess failed to terminate cleanly.

To simplify joining of multiple processes, a new testing function join_processes was added, where all processes in the list will be joined with a common timeout, if the total time elapsed is longer than the timeout the process will still be joined but wait will be non-positive, meaning join returns immediately and the process is later terminated with terminate_process.

@pentschev pentschev requested a review from a team as a code owner October 16, 2024 11:11
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Looks fine other than one question about potential for negative timeouts?

"""
start = time.monotonic()
for p in processes:
t = timeout - (time.monotonic() - start)
Copy link
Contributor

Choose a reason for hiding this comment

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

question: What happens if the timeout you pass to join is negative? I suspect that can occur since joining a process and getting to the next iteration can take a non-zero amount of time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I tested that and it behaves the same as timeout=0, essentially returns immediately, although the documentation only mentions positive timeouts (to me implies any non-positive number is indeed expected to return immediately), do you think we should attempt to make the value always non-negative in case this is not documented because it's undefined behavior/implementation-dependent?

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