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

gh-100228: Warn from os.fork() if other threads exist. #100229

Merged
merged 20 commits into from
Dec 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions Include/internal/pycore_global_objects_fini_generated.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Include/internal/pycore_global_strings.h
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ struct _Py_global_strings {
STRUCT_FOR_ID(__xor__)
STRUCT_FOR_ID(_abc_impl)
STRUCT_FOR_ID(_abstract_)
STRUCT_FOR_ID(_active)
STRUCT_FOR_ID(_annotation)
STRUCT_FOR_ID(_anonymous_)
STRUCT_FOR_ID(_argtypes_)
Expand All @@ -239,6 +240,7 @@ struct _Py_global_strings {
STRUCT_FOR_ID(_initializing)
STRUCT_FOR_ID(_is_text_encoding)
STRUCT_FOR_ID(_length_)
STRUCT_FOR_ID(_limbo)
STRUCT_FOR_ID(_lock_unlock_module)
STRUCT_FOR_ID(_loop)
STRUCT_FOR_ID(_needs_com_addref_)
Expand Down
2 changes: 2 additions & 0 deletions Include/internal/pycore_runtime_init_generated.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions Include/internal/pycore_unicodeobject_generated.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 15 additions & 16 deletions Lib/test/fork_wait.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import threading
from test import support
from test.support import threading_helper
import warnings


LONGSLEEP = 2
Expand Down Expand Up @@ -63,19 +64,17 @@ def test_wait(self):

prefork_lives = self.alive.copy()

if sys.platform in ['unixware7']:
cpid = os.fork1()
else:
cpid = os.fork()

if cpid == 0:
# Child
time.sleep(LONGSLEEP)
n = 0
for key in self.alive:
if self.alive[key] != prefork_lives[key]:
n += 1
os._exit(n)
else:
# Parent
self.wait_impl(cpid, exitcode=0)
# Ignore the warning about fork with threads.
with warnings.catch_warnings(category=DeprecationWarning,
action="ignore"):
if (cpid := os.fork()) == 0:
# Child
time.sleep(LONGSLEEP)
n = 0
for key in self.alive:
if self.alive[key] != prefork_lives[key]:
n += 1
os._exit(n)
else:
# Parent
self.wait_impl(cpid, exitcode=0)
28 changes: 28 additions & 0 deletions Lib/test/test_os.py
Original file line number Diff line number Diff line change
Expand Up @@ -4577,6 +4577,34 @@ def test_fork(self):
assert_python_ok("-c", code)
assert_python_ok("-c", code, PYTHONMALLOC="malloc_debug")

@unittest.skipUnless(sys.platform in ("linux", "darwin"),
"Only Linux and macOS detect this today.")
def test_fork_warns_when_non_python_thread_exists(self):
code = """if 1:
import os, threading, warnings
from _testcapi import _spawn_pthread_waiter, _end_spawned_pthread
_spawn_pthread_waiter()
try:
with warnings.catch_warnings(record=True) as ws:
warnings.filterwarnings(
"always", category=DeprecationWarning)
if os.fork() == 0:
assert not ws, f"unexpected warnings in child: {ws}"
os._exit(0) # child
else:
assert ws[0].category == DeprecationWarning, ws[0]
assert 'fork' in str(ws[0].message), ws[0]
# Waiting allows an error in the child to hit stderr.
exitcode = os.wait()[1]
assert exitcode == 0, f"child exited {exitcode}"
assert threading.active_count() == 1, threading.enumerate()
finally:
_end_spawned_pthread()
"""
_, out, err = assert_python_ok("-c", code, PYTHONOPTIMIZE='0')
self.assertEqual(err.decode("utf-8"), "")
self.assertEqual(out.decode("utf-8"), "")


# Only test if the C version is provided, otherwise TestPEP519 already tested
# the pure Python implementation.
Expand Down
13 changes: 8 additions & 5 deletions Lib/test/test_thread.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from test.support import threading_helper
import _thread as thread
import time
import warnings
import weakref

from test import lock_tests
Expand Down Expand Up @@ -238,11 +239,13 @@ def test_forkinthread(self):
def fork_thread(read_fd, write_fd):
nonlocal pid

# fork in a thread
pid = os.fork()
if pid:
# parent process
return
# Ignore the warning about fork with threads.
with warnings.catch_warnings(category=DeprecationWarning,
action="ignore"):
# fork in a thread (DANGER, undefined per POSIX)
if (pid := os.fork()):
# parent process
return

# child process
try:
Expand Down
110 changes: 65 additions & 45 deletions Lib/test/test_threading.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import signal
import textwrap
import traceback
import warnings

from unittest import mock
from test import lock_tests
Expand Down Expand Up @@ -563,7 +564,7 @@ def test_dummy_thread_after_fork(self):
# Issue #14308: a dummy thread in the active list doesn't mess up
# the after-fork mechanism.
code = """if 1:
import _thread, threading, os, time
import _thread, threading, os, time, warnings

def background_thread(evt):
# Creates and registers the _DummyThread instance
Expand All @@ -575,11 +576,16 @@ def background_thread(evt):
_thread.start_new_thread(background_thread, (evt,))
evt.wait()
assert threading.active_count() == 2, threading.active_count()
if os.fork() == 0:
assert threading.active_count() == 1, threading.active_count()
os._exit(0)
else:
os.wait()
with warnings.catch_warnings(record=True) as ws:
warnings.filterwarnings(
"always", category=DeprecationWarning)
if os.fork() == 0:
assert threading.active_count() == 1, threading.active_count()
os._exit(0)
else:
assert ws[0].category == DeprecationWarning, ws[0]
assert 'fork' in str(ws[0].message), ws[0]
os.wait()
"""
_, out, err = assert_python_ok("-c", code)
self.assertEqual(out, b'')
Expand All @@ -598,13 +604,15 @@ def test_is_alive_after_fork(self):
for i in range(20):
t = threading.Thread(target=lambda: None)
t.start()
pid = os.fork()
if pid == 0:
os._exit(11 if t.is_alive() else 10)
else:
t.join()
# Ignore the warning about fork with threads.
with warnings.catch_warnings(category=DeprecationWarning,
action="ignore"):
if (pid := os.fork()) == 0:
os._exit(11 if t.is_alive() else 10)
else:
t.join()

support.wait_process(pid, exitcode=10)
support.wait_process(pid, exitcode=10)

def test_main_thread(self):
main = threading.main_thread()
Expand Down Expand Up @@ -645,29 +653,34 @@ def test_main_thread_after_fork(self):
@unittest.skipUnless(hasattr(os, 'waitpid'), "test needs os.waitpid()")
def test_main_thread_after_fork_from_nonmain_thread(self):
code = """if 1:
import os, threading, sys
import os, threading, sys, warnings
from test import support

def func():
pid = os.fork()
if pid == 0:
main = threading.main_thread()
print(main.name)
print(main.ident == threading.current_thread().ident)
print(main.ident == threading.get_ident())
# stdout is fully buffered because not a tty,
# we have to flush before exit.
sys.stdout.flush()
else:
support.wait_process(pid, exitcode=0)
with warnings.catch_warnings(record=True) as ws:
warnings.filterwarnings(
"always", category=DeprecationWarning)
pid = os.fork()
if pid == 0:
main = threading.main_thread()
print(main.name)
print(main.ident == threading.current_thread().ident)
print(main.ident == threading.get_ident())
# stdout is fully buffered because not a tty,
# we have to flush before exit.
sys.stdout.flush()
else:
assert ws[0].category == DeprecationWarning, ws[0]
assert 'fork' in str(ws[0].message), ws[0]
support.wait_process(pid, exitcode=0)

th = threading.Thread(target=func)
th.start()
th.join()
"""
_, out, err = assert_python_ok("-c", code)
data = out.decode().replace('\r', '')
self.assertEqual(err, b"")
self.assertEqual(err.decode('utf-8'), "")
self.assertEqual(data, "Thread-1 (func)\nTrue\nTrue\n")

def test_main_thread_during_shutdown(self):
Expand Down Expand Up @@ -1173,15 +1186,18 @@ def do_fork_and_wait():
else:
os._exit(50)

# start a bunch of threads that will fork() child processes
threads = []
for i in range(16):
t = threading.Thread(target=do_fork_and_wait)
threads.append(t)
t.start()
# Ignore the warning about fork with threads.
with warnings.catch_warnings(category=DeprecationWarning,
action="ignore"):
# start a bunch of threads that will fork() child processes
threads = []
for i in range(16):
t = threading.Thread(target=do_fork_and_wait)
threads.append(t)
t.start()

for t in threads:
t.join()
for t in threads:
t.join()

@support.requires_fork()
def test_clear_threads_states_after_fork(self):
Expand All @@ -1194,18 +1210,22 @@ def test_clear_threads_states_after_fork(self):
threads.append(t)
t.start()

pid = os.fork()
if pid == 0:
# check that threads states have been cleared
if len(sys._current_frames()) == 1:
os._exit(51)
else:
os._exit(52)
else:
support.wait_process(pid, exitcode=51)

for t in threads:
t.join()
try:
# Ignore the warning about fork with threads.
with warnings.catch_warnings(category=DeprecationWarning,
action="ignore"):
pid = os.fork()
if pid == 0:
# check that threads states have been cleared
if len(sys._current_frames()) == 1:
os._exit(51)
else:
os._exit(52)
else:
support.wait_process(pid, exitcode=51)
finally:
for t in threads:
t.join()


class SubinterpThreadingTests(BaseTestCase):
Expand Down
2 changes: 2 additions & 0 deletions Lib/threading.py
Original file line number Diff line number Diff line change
Expand Up @@ -1490,6 +1490,8 @@ def active_count():
enumerate().

"""
# NOTE: if the logic in here ever changes, update Modules/posixmodule.c
# warn_about_fork_with_threads() to match.
with _active_limbo_lock:
return len(_active) + len(_limbo)

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
A :exc:`DeprecationWarning` may be raised when :func:`os.fork()` or
:func:`os.forkpty()` is called from multi-threaded processes. Forking
with threads is unsafe and can cause deadlocks, crashes and subtle
problems. Lack of a warning does not indicate that the fork call was
actually safe, as Python may not be aware of all threads.
Loading