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

lib: Attach stdout to child only if --log=stdout and stdout FD is a tty #16738

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

odivlad
Copy link
Contributor

@odivlad odivlad commented Sep 3, 2024

We've faced with a regression after we moved from frr 8.5 to a newer versions (9.0, 9.1, 10.0, 10.1).

In our automation we start frr from python code using subprocess like this:

subprocess.run(
    ["/usr/lib/frr/frrinit.sh", "start"],
    stdout=subprocess.PIPE,
    stderr=subprocess.PIPE,
)

This call hangs in poll() for stdout FD indefinitely and frrinit.sh hangs in zombie state unless we kill subprocess.run() call.
If we remove stdout capturing from subprocess.run() call (make associated to stdout FD a tty FD), things go well and process terminates correctly with no hangs and zombies.
This commit fixes this situation where stdout of a process started in a daemon mode was attached to a calling process regardless of presence --log option.

If this patch is acceptable, it's worth backporting down to 9.0 branch, where the initial commit 4d28aea appeared.

Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

Could you fix the styling?

@odivlad
Copy link
Contributor Author

odivlad commented Sep 4, 2024

@ton31337, sure. Done.

@ton31337
Copy link
Member

ton31337 commented Sep 4, 2024

@odivlad odivlad changed the title lib: Attach stdout to child only if --log=stdout and stdout FD is a tty. lib: Attach stdout to child only if --log=stdout and stdout FD is a tty Sep 4, 2024
@odivlad
Copy link
Contributor Author

odivlad commented Sep 4, 2024

@ton31337, done.
It was not obviously that git clang-format should be called twice.
Additionally removed a period symbol from the end of commit summary.

lib/libfrr.c Outdated Show resolved Hide resolved
Prior to this commit stdout of a process started in a daemon mode was
attached to a calling process.
As a result a calling process hung for infinity.

Signed-off-by: Vladislav Odintsov <[email protected]>
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

LGTM

@ton31337 ton31337 merged commit 74f1285 into FRRouting:master Sep 19, 2024
11 checks passed
@odivlad odivlad deleted the fix-stdout-fork branch September 24, 2024 13:50
@odivlad
Copy link
Contributor Author

odivlad commented Sep 24, 2024

@ton31337, this change seems to be qualified as a bugfix, so would you consider to backport it down to supported branches?

@odivlad
Copy link
Contributor Author

odivlad commented Oct 1, 2024

@ton31337, it's just gentle ping about backport.

@ton31337
Copy link
Member

ton31337 commented Oct 1, 2024

@Mergifyio backport stable/10.1 stable/10.0 stable/9.1

Copy link

mergify bot commented Oct 1, 2024

backport stable/10.1 stable/10.0 stable/9.1

✅ Backports have been created

ton31337 added a commit that referenced this pull request Oct 1, 2024
lib: Attach stdout to child only if --log=stdout and stdout FD is a tty (backport #16738)
donaldsharp added a commit that referenced this pull request Oct 1, 2024
lib: Attach stdout to child only if --log=stdout and stdout FD is a tty (backport #16738)
donaldsharp added a commit that referenced this pull request Oct 1, 2024
lib: Attach stdout to child only if --log=stdout and stdout FD is a tty (backport #16738)
@odivlad
Copy link
Contributor Author

odivlad commented Oct 2, 2024

Thanks, @ton31337 and @donaldsharp!

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.

3 participants