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

Prevent stdio deadlock in forked children #79522

Merged
merged 8 commits into from
Dec 6, 2022

Conversation

nitzmahone
Copy link
Member

@nitzmahone nitzmahone commented Dec 2, 2022

SUMMARY
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

display.py

ADDITIONAL INFORMATION

@ansibot
Copy link
Contributor

ansibot commented Dec 2, 2022

The test ansible-test sanity --test shebang [explain] failed with 1 error:

test/integration/targets/fork_safe_stdio/runme.sh:1:1: unexpected non-module shebang: b'#!/bin/bash'

click here for bot help

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.15 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. labels Dec 2, 2022
@nitzmahone nitzmahone force-pushed the fork_safe_stdio branch 2 times, most recently from 0bf86db to 729222a Compare December 4, 2022 01:50
@nitzmahone nitzmahone changed the title Add integration tests for stdio deadlock Prevent stdio deadlock in forked children Dec 5, 2022
@nitzmahone nitzmahone removed the needs_triage Needs a first human triage before being processed. label Dec 5, 2022
lib/ansible/utils/display.py Outdated Show resolved Hide resolved
@nitzmahone nitzmahone marked this pull request as ready for review December 5, 2022 19:02
@ansibot ansibot added test This PR relates to tests. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Dec 5, 2022
@ansibot
Copy link
Contributor

ansibot commented Dec 6, 2022

The test ansible-test sanity --test pep8 [explain] failed with 11 errors:

test/integration/targets/fork_safe_stdio/vendored_pty.py:28:1: E302: expected 2 blank lines, found 1
test/integration/targets/fork_safe_stdio/vendored_pty.py:40:1: E302: expected 2 blank lines, found 1
test/integration/targets/fork_safe_stdio/vendored_pty.py:56:1: E302: expected 2 blank lines, found 1
test/integration/targets/fork_safe_stdio/vendored_pty.py:68:1: E302: expected 2 blank lines, found 1
test/integration/targets/fork_safe_stdio/vendored_pty.py:86:1: E302: expected 2 blank lines, found 1
test/integration/targets/fork_safe_stdio/vendored_pty.py:126:1: E302: expected 2 blank lines, found 1
test/integration/targets/fork_safe_stdio/vendored_pty.py:132:1: E302: expected 2 blank lines, found 1
test/integration/targets/fork_safe_stdio/vendored_pty.py:136:1: E302: expected 2 blank lines, found 1
test/integration/targets/fork_safe_stdio/vendored_pty.py:154:27: E114: indentation is not a multiple of 4 (comment)
test/integration/targets/fork_safe_stdio/vendored_pty.py:154:27: E116: unexpected indentation (comment)
test/integration/targets/fork_safe_stdio/vendored_pty.py:165:1: E302: expected 2 blank lines, found 1

click here for bot help

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 6, 2022
* demonstrates the underlying issue behind ansible/ansible-runner#1164
* background threads writing to stdout/stderr can cause children to deadlock if a thread in the parent holds the internal lock on the BufferedWriter wrapper
* monkeypatch stdout/stderr during display startup to require a mutex lock with fork(); this ensures no background threads can hold the lock during a fork operation
* patch with something that looks more "real" to avoid warning messages in import sanity (thanks sivel!)
@mattclay
Copy link
Member

mattclay commented Dec 6, 2022

A sanity skip line is needed for the vendored file, like this one:

test/integration/targets/ansible-test-no-tty/ansible_collections/ns/col/vendored_pty.py pep8!skip # vendored code

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 6, 2022
* works with pty wrapper script
* still skipping macos since timeout is missing
@nitzmahone nitzmahone merged commit 1424484 into ansible:devel Dec 6, 2022
nitzmahone added a commit to nitzmahone/ansible that referenced this pull request Dec 6, 2022
* background threads writing to stdout/stderr can cause children to deadlock if a thread in the parent holds the internal lock on the BufferedWriter wrapper
* prevent writes to std handles during fork by monkeypatching stdout/stderr during display startup to require a mutex lock with fork(); this ensures no background threads can hold the lock during a fork operation
* add integration test that fails reliably on Linux without this fix

(cherry picked from commit 1424484)
anweshadas pushed a commit to anweshadas/ansible that referenced this pull request Dec 6, 2022
* background threads writing to stdout/stderr can cause children to deadlock if a thread in the parent holds the internal lock on the BufferedWriter wrapper
* prevent writes to std handles during fork by monkeypatching stdout/stderr during display startup to require a mutex lock with fork(); this ensures no background threads can hold the lock during a fork operation
* add integration test that fails reliably on Linux without this fix
nitzmahone added a commit that referenced this pull request Dec 6, 2022
* background threads writing to stdout/stderr can cause children to deadlock if a thread in the parent holds the internal lock on the BufferedWriter wrapper
* prevent writes to std handles during fork by monkeypatching stdout/stderr during display startup to require a mutex lock with fork(); this ensures no background threads can hold the lock during a fork operation
* add integration test that fails reliably on Linux without this fix

(cherry picked from commit 1424484)
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Dec 8, 2022
v2.14.1
=======

Minor Changes
-------------
- ansible-test - Improve consistency of executed ``pylint`` commands by making the plugins ordered.

Bugfixes
--------
- Fixes leftover _valid_attrs usage.
- ansible-galaxy - make initial call to Galaxy server on-demand only when installing, getting info about, and listing roles.
- copy module will no longer move 'non files' set as src when remote_src=true.
- display - reduce risk of post-fork output deadlocks (ansible/ansible#79522)
- jinja2_native: preserve quotes in strings (ansible/ansible#79083)
- updated error messages to include 'acl' and not just mode changes when failing to set required permissions on remote.
@ansible ansible locked and limited conversation to collaborators Dec 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.15 bug This issue/PR relates to a bug. has_issue test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants