Skip to content

Revert "GH-120754: Add a strace helper and test set of syscalls for o… #123303

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 24, 2024
Merged

Revert "GH-120754: Add a strace helper and test set of syscalls for o… #123303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 0 additions & 166 deletions Lib/test/support/strace_helper.py
View file
Open in desktop

This file was deleted.

93 changes: 1 addition & 92 deletions Lib/test/test_fileio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

from test.support import (
cpython_only, swap_attr, gc_collect, is_emscripten, is_wasi,
infinite_recursion, strace_helper
infinite_recursion,
)
from test.support.os_helper import (
TESTFN, TESTFN_ASCII, TESTFN_UNICODE, make_bad_fd,
Expand All @@ -24,9 +24,6 @@
import _pyio # Python implementation of io


_strace_flags=["--trace=%file,%desc"]


class AutoFileTests:
# file tests for which a test file is automatically set up

Expand Down Expand Up @@ -362,94 +359,6 @@ def testErrnoOnClosedReadinto(self, f):
a = array('b', b'x'*10)
f.readinto(a)

@strace_helper.requires_strace()
def test_syscalls_read(self):
"""Check that the set of system calls produced by the I/O stack is what
is expected for various read cases.

It's expected as bits of the I/O implementation change, this will need
to change. The goal is to catch changes that unintentionally add
additional systemcalls (ex. additional calls have been looked at in
bpo-21679 and gh-120754).
"""
self.f.write(b"Hello, World!")
self.f.close()


def check_readall(name, code, prelude="", cleanup=""):
with self.subTest(name=name):
syscalls = strace_helper.get_syscalls(code, _strace_flags,
prelude=prelude,
cleanup=cleanup)

# There are a number of related syscalls used to implement
# behaviors in a libc (ex. fstat, newfstatat, open, openat).
# Allow any that use the same substring.
def count_similarname(name):
return len([sc for sc in syscalls if name in sc])

# Should open and close the file exactly once
self.assertEqual(count_similarname('open'), 1)
self.assertEqual(count_similarname('close'), 1)

# Should only have one fstat (bpo-21679, gh-120754)
self.assertEqual(count_similarname('fstat'), 1)


# "open, read, close" file using different common patterns.
check_readall(
"open builtin with default options",
f"""
f = open('{TESTFN}')
f.read()
f.close()
"""
)

check_readall(
"open in binary mode",
f"""
f = open('{TESTFN}', 'rb')
f.read()
f.close()
"""
)

check_readall(
"open in text mode",
f"""
f = open('{TESTFN}', 'rt')
f.read()
f.close()
"""
)

check_readall(
"pathlib read_bytes",
"p.read_bytes()",
prelude=f"""from pathlib import Path; p = Path("{TESTFN}")"""

)

check_readall(
"pathlib read_text",
"p.read_text()",
prelude=f"""from pathlib import Path; p = Path("{TESTFN}")"""
)

# Focus on just `read()`.
calls = strace_helper.get_syscalls(
prelude=f"f = open('{TESTFN}')",
code="f.read()",
cleanup="f.close()",
strace_flags=_strace_flags
)
# One to read all the bytes
# One to read the EOF and get a size 0 return.
self.assertEqual(calls.count("read"), 2)



class CAutoFileTests(AutoFileTests, unittest.TestCase):
FileIO = _io.FileIO
modulename = '_io'
Expand Down
53 changes: 32 additions & 21 deletions Lib/test/test_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
from test.support import check_sanitizer
from test.support import import_helper
from test.support import os_helper
from test.support import strace_helper
from test.support import warnings_helper
from test.support.script_helper import assert_python_ok
import subprocess
Expand Down Expand Up @@ -3416,33 +3415,44 @@ def __del__(self):

@unittest.skipIf(not sysconfig.get_config_var("HAVE_VFORK"),
"vfork() not enabled by configure.")
@strace_helper.requires_strace()
@unittest.skipIf(sys.platform != "linux", "Linux only, requires strace.")
@mock.patch("subprocess._USE_POSIX_SPAWN", new=False)
def test_vfork_used_when_expected(self):
# This is a performance regression test to ensure we default to using
# vfork() when possible.
# Technically this test could pass when posix_spawn is used as well
# because libc tends to implement that internally using vfork. But
# that'd just be testing a libc+kernel implementation detail.

# Are intersted in the system calls:
# clone,clone2,clone3,fork,vfork,exit,exit_group
# Unfortunately using `--trace` with that list to strace fails because
# not all are supported on all platforms (ex. clone2 is ia64 only...)
# So instead use `%process` which is recommended by strace, and contains
# the above.
strace_binary = "/usr/bin/strace"
# The only system calls we are interested in.
strace_filter = "--trace=clone,clone2,clone3,fork,vfork,exit,exit_group"
true_binary = "/bin/true"
strace_args = ["--trace=%process"]
strace_command = [strace_binary, strace_filter]

try:
does_strace_work_process = subprocess.run(
strace_command + [true_binary],
stderr=subprocess.PIPE,
stdout=subprocess.DEVNULL,
)
rc = does_strace_work_process.returncode
stderr = does_strace_work_process.stderr
except OSError:
rc = -1
stderr = ""
if rc or (b"+++ exited with 0 +++" not in stderr):
self.skipTest("strace not found or not working as expected.")

with self.subTest(name="default_is_vfork"):
vfork_result = strace_helper.strace_python(
f"""\
import subprocess
subprocess.check_call([{true_binary!r}])""",
strace_args
vfork_result = assert_python_ok(
"-c",
textwrap.dedent(f"""\
import subprocess
subprocess.check_call([{true_binary!r}])"""),
__run_using_command=strace_command,
)
# Match both vfork() and clone(..., flags=...|CLONE_VFORK|...)
self.assertRegex(vfork_result.event_bytes, br"(?i)vfork")
self.assertRegex(vfork_result.err, br"(?i)vfork")
# Do NOT check that fork() or other clones did not happen.
# If the OS denys the vfork it'll fallback to plain fork().

Expand All @@ -3455,20 +3465,21 @@ def test_vfork_used_when_expected(self):
("setgroups", "", "extra_groups=[]", True),
):
with self.subTest(name=sub_name):
non_vfork_result = strace_helper.strace_python(
f"""\
non_vfork_result = assert_python_ok(
"-c",
textwrap.dedent(f"""\
import subprocess
{preamble}
try:
subprocess.check_call(
[{true_binary!r}], **dict({sp_kwarg}))
except PermissionError:
if not {expect_permission_error}:
raise""",
strace_args
raise"""),
__run_using_command=strace_command,
)
# Ensure neither vfork() or clone(..., flags=...|CLONE_VFORK|...).
self.assertNotRegex(non_vfork_result.event_bytes, br"(?i)vfork")
self.assertNotRegex(non_vfork_result.err, br"(?i)vfork")


@unittest.skipUnless(mswindows, "Windows specific tests")
Expand Down
1 change: 0 additions & 1 deletion Misc/ACKS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1164,7 +1164,6 @@ Grzegorz Makarewicz
David Malcolm
Greg Malcolm
William Mallard
Cody Maloney
Ken Manheimer
Vladimir Marangozov
Colin Marc
Expand Down
Loading