Skip to content

Bound the reader-thread joins in Command - #3

Merged
patrsc merged 2 commits into
BeamNG:mainfrom
ImLp:main
Sep 17, 2026
Merged

patrsc merged 2 commits into
BeamNG:mainfrom
ImLp:main

Conversation

@ImLp

@ImLp ImLp commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Problem

Command.wait() joins its stdout and stderr reader threads with no timeout. A grandchild process that inherited the pipes keeps them open after the child exits, so the readers never see EOF and wait() blocks indefinitely, well past its own timeout argument.

Fix

  • Both joins now honour reader_join_timeout, default 10s.
  • readers_abandoned records that a join gave up, so a caller can tell.
  • __exit__'s error path bounds the same two joins.

Scope

The reader threads are daemons and append to their output lists line by line, so giving up on them loses only trailing output from a process that is already being terminated. A clean exit still drains fully.

Testing

Two cases added to examples/test_command.py: a child that leaves a grandchild on its stdout made wait(timeout=5) return after 12.1s before this change and 2.5s after, and a clean exit still drains with readers_abandoned False. Full example suite: 32 passed, 3 skipped.

Luis Placid added 2 commits September 16, 2026 10:30
A grandchild that inherits the pipes keeps them open after the child exits, so an unbounded join waited on a process nobody tracked.
@cla-bot

cla-bot Bot commented Sep 16, 2026

Copy link
Copy Markdown

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Luis Placid.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@patrsc
patrsc merged commit 080fe87 into BeamNG:main Sep 17, 2026
0 of 6 checks passed
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