Skip to content

Commit

Permalink
Fix the remainder of the 3.11 subprocess tests; try to stop skipping …
Browse files Browse the repository at this point in the history
…the tree format tests on mac.

subprocess needed a new check added to check_output, and needed process_group implemented.

because all the leakcheck tests finished successfully after the last commit, and because it sure looks like a refcounting issues in the tree formatting tests (leaked greenlets causing the numbering to be off), try enabling them for this run.
  • Loading branch information
jamadden committed Oct 7, 2022
1 parent dbb55b1 commit 80e63fa
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 28 deletions.
36 changes: 28 additions & 8 deletions src/gevent/subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
from gevent._compat import PY36
from gevent._compat import PY37
from gevent._compat import PY38
from gevent._compat import PY311
from gevent._compat import PYPY
from gevent._compat import reraise
from gevent._compat import fsdecode
Expand Down Expand Up @@ -210,6 +211,21 @@ def _use_posix_spawn():
'_USE_POSIX_SPAWN',
])

if PY311:
# Python 3.11 added some module-level attributes to control the
# use of vfork. The docs specifically say that you should not try to read
# them, only set them, so we don't provide them.
#
# Python 3.11 also added a test, test_surrogates_error_message, that behaves
# differently based on whether or not the pure python implementation of forking
# is in use, or the one written in C from _posixsubprocess. Obviously we don't call
# that, so we need to make us look like a pure python version; it checks that this attribute
# is none for that.
_fork_exec = None
__implements__.extend([
'_fork_exec',
])

actually_imported = copy_globals(__subprocess__, globals(),
only_names=__imports__,
ignore_missing_names=True)
Expand Down Expand Up @@ -249,6 +265,7 @@ def _use_posix_spawn():
__all__.append(_x)



mswindows = sys.platform == 'win32'
if mswindows:
import msvcrt # pylint: disable=import-error
Expand Down Expand Up @@ -376,10 +393,14 @@ def check_output(*popenargs, **kwargs):
.. versionchanged:: 1.2a1
The ``input`` keyword argument is now accepted on all supported
versions of Python, not just Python 3
.. versionchanged:: NEXT
Passing the ``check`` keyword argument is forbidden, just as in Python 3.11.
"""
timeout = kwargs.pop('timeout', None)
if 'stdout' in kwargs:
raise ValueError('stdout argument not allowed, it will be overridden.')
if 'check' in kwargs:
raise ValueError('check argument not allowed, it will be overridden.')
if 'input' in kwargs:
if 'stdin' in kwargs:
raise ValueError('stdin and input arguments may not both be used.')
Expand Down Expand Up @@ -663,9 +684,6 @@ def __init__(self, args,
pipesize=-1,
# Added in 3.11
process_group=None,
# 3.11: check added, but not documented as new (at least as-of
# 3.11rc2)
check=None,
# gevent additions
threadpool=None):

Expand Down Expand Up @@ -828,7 +846,7 @@ def __init__(self, args,
errread, errwrite,
restore_signals,
gid, gids, uid, umask,
start_new_session)
start_new_session, process_group)
except:
# Cleanup if the child failed starting.
# (gevent: New in python3, but reported as gevent bug in #347.
Expand Down Expand Up @@ -1197,7 +1215,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
errread, errwrite,
unused_restore_signals,
unused_gid, unused_gids, unused_uid, unused_umask,
unused_start_new_session):
unused_start_new_session, unused_process_group):
"""Execute program (MS Windows version)"""
# pylint:disable=undefined-variable
assert not pass_fds, "pass_fds not supported on Windows."
Expand Down Expand Up @@ -1606,7 +1624,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
errread, errwrite,
restore_signals,
gid, gids, uid, umask,
start_new_session):
start_new_session, process_group):
"""Execute program (POSIX version)"""

if PY3 and isinstance(args, (str, bytes)):
Expand Down Expand Up @@ -1740,7 +1758,8 @@ def _dup2(existing, desired):
os.setregid(gid, gid)
if uid:
os.setreuid(uid, uid)

if process_group is not None:
os.setpgid(0, process_group)
if preexec_fn:
preexec_fn()

Expand Down Expand Up @@ -2024,13 +2043,14 @@ def run(*popenargs, **kwargs):

return CompletedProcess(process.args, retcode, stdout, stderr)

def _gevent_did_monkey_patch(*_args):
def _gevent_did_monkey_patch(target_module, *_args, **_kwargs):
# Beginning on 3.8 on Mac, the 'spawn' method became the default
# start method. That doesn't fire fork watchers and we can't
# easily patch to make it do so: multiprocessing uses the private
# c accelerated _subprocess module to implement this. Instead we revert
# back to using fork.
from gevent._compat import MAC

if MAC:
import multiprocessing
if hasattr(multiprocessing, 'set_start_method'):
Expand Down
22 changes: 2 additions & 20 deletions src/gevent/tests/test__util.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,24 +23,9 @@ class MyLocal(local.local):
def __init__(self, foo):
self.foo = foo


MAC_CI_SKIP_MSG = """
Starting 20221007, these sometimes fail on GitHubActions, on
at least 3.7 and 3.10. They do not fail for me on my mac
locally.
This should be investigated and reproduced. I expect it's a
timing issue; it may also be a reference leak, so
test this again when the two threading tests that involve ref leaks
are re-enabled and fixed. I'm OK disabling on mac for now since we have the
linux and appveyor coverage still.
""" # Lines that fail are marked XXX: Mac CI
# See for example https://github.com/gevent/gevent/actions/runs/3205900356/jobs/5239610262

@greentest.skipOnPyPy("5.10.x is *very* slow formatting stacks")
class TestFormat(greentest.TestCase):

@greentest.skipOnMacOnCI(MAC_CI_SKIP_MSG)
def test_basic(self):
lines = util.format_run_info()

Expand All @@ -49,11 +34,10 @@ def test_basic(self):
self.assertIn('Greenlets', value)

# because it's a raw greenlet, we have no data for it.
self.assertNotIn("Spawned at", value) # XXX: Mac CI
self.assertNotIn("Spawned at", value)
self.assertNotIn("Parent greenlet", value)
self.assertNotIn("Spawn Tree Locals", value)

@greentest.skipOnMacOnCI(MAC_CI_SKIP_MSG)
def test_with_Greenlet(self):
rl = local.local()
rl.foo = 1
Expand Down Expand Up @@ -82,7 +66,7 @@ def root():
self.assertIn('MyLocal', value)
self.assertIn("Printer", value) # The name is printed
# Empty locals should not be printed
self.assertNotIn('{}', value) # XXX: Mac CI This is a weird one.
self.assertNotIn('{}', value)

@greentest.skipOnPyPy("See TestFormat")
class TestTree(greentest.TestCase):
Expand Down Expand Up @@ -172,7 +156,6 @@ def _normalize_tree_format(self, value):
return value

@greentest.ignores_leakcheck
@greentest.skipOnMacOnCI(MAC_CI_SKIP_MSG)
def test_tree(self):
with gevent.get_hub().ignoring_expected_test_error():
tree, str_tree, tree_format = self._build_tree()
Expand Down Expand Up @@ -210,7 +193,6 @@ def test_tree(self):
Parent: <HUB>
""".strip()
self.assertEqual(expected, value)
# XXX mac ci: This may be a reference leak.

@greentest.ignores_leakcheck
def test_tree_no_track(self):
Expand Down

0 comments on commit 80e63fa

Please sign in to comment.