[gentoo-commits] proj/portage:master commit in: lib/portage/tests/ebuild/, lib/portage/, lib/_emerge/

[gentoo-commits] proj/portage:master commit in: lib/portage/tests/ebuild/, lib/portage/, lib/_emerge/

[Zac Medico]

commit:     052a4076cdf29fde8481646c636190d9db35f0ff
Author:     Zac Medico <zmedico <AT> gentoo <DOT> org>
AuthorDate: Sun Feb  4 08:08:01 2024 +0000
Commit:     Zac Medico <zmedico <AT> gentoo <DOT> org>
CommitDate: Sun Feb  4 08:10:12 2024 +0000
URL:        https://gitweb.gentoo.org/proj/portage.git/commit/?id=052a4076

Revert "process.spawn: Use multiprocessing.Process for returnproc"

This reverts commit 305612d1b04aa06d3d1a1c8b51d046a644742fd5.
It triggered a "Bad file descriptor" during the instprep
phase as reported in bug 923755.

Bug: https://bugs.gentoo.org/923755
Signed-off-by: Zac Medico <zmedico <AT> gentoo.org>

 lib/_emerge/SpawnProcess.py                        |  4 +-
 lib/portage/process.py                             | 72 ++++------------------
 lib/portage/tests/ebuild/test_doebuild_fd_pipes.py |  6 +-
 3 files changed, 15 insertions(+), 67 deletions(-)

diff --git a/lib/_emerge/SpawnProcess.py b/lib/_emerge/SpawnProcess.py
index 5e582e3322..7f4a23892b 100644
--- a/lib/_emerge/SpawnProcess.py
+++ b/lib/_emerge/SpawnProcess.py
@@ -224,9 +224,7 @@ class SpawnProcess(SubProcess):
         got_pty, master_fd, slave_fd = _create_pty_or_pipe(copy_term_size=stdout_pipe)
         return (master_fd, slave_fd)
 
-    def _spawn(
-        self, args: list[str], **kwargs
-    ) -> portage.process.MultiprocessingProcess:
+    def _spawn(self, args: list[str], **kwargs) -> portage.process.Process:
         spawn_func = portage.process.spawn
 
         if self._selinux_type is not None:

diff --git a/lib/portage/process.py b/lib/portage/process.py
index d64ffa924f..01426179d7 100644
--- a/lib/portage/process.py
+++ b/lib/portage/process.py
@@ -19,7 +19,7 @@ import warnings
 
 from dataclasses import dataclass
 from functools import lru_cache
-from typing import Any, Optional, Callable, Union
+from typing import Any, Optional, Callable
 
 from portage import os
 from portage import _encodings
@@ -28,7 +28,6 @@ import portage
 
 portage.proxy.lazyimport.lazyimport(
     globals(),
-    "portage.util._async.ForkProcess:ForkProcess",
     "portage.util._eventloop.global_event_loop:global_event_loop",
     "portage.util.futures:asyncio",
     "portage.util:dump_traceback,writemsg,writemsg_level",
@@ -297,19 +296,12 @@ class AbstractProcess:
 
 class Process(AbstractProcess):
     """
-    An object that wraps OS processes which do not have an
-    associated multiprocessing.Process instance. Ultimately,
-    we need to stop using os.fork() to create these processes
-    because it is unsafe for threaded processes as discussed
-    in https://github.com/python/cpython/issues/84559.
-
-    Note that if subprocess.Popen is used without pass_fds
-    or preexec_fn parameters, then it avoids using os.fork()
-    by instead using posix_spawn. This approach is not used
-    by spawn because it needs to execute python code prior
-    to exec, so it instead uses multiprocessing.Process,
-    which only uses os.fork() when the multiprocessing start
-    method is fork.
+    An object that wraps OS processes created by spawn.
+    In the future, spawn will return objects of a different type
+    but with a compatible interface to this class, in order
+    to encapsulate implementation-dependent objects like
+    multiprocessing.Process which are designed to manage
+    the process lifecycle and need to persist until it exits.
     """
 
     def __init__(self, pid: int):
@@ -469,7 +461,7 @@ def spawn(
     unshare_mount=False,
     unshare_pid=False,
     warn_on_large_env=False,
-) -> Union[int, MultiprocessingProcess, list[int]]:
+):
     """
     Spawns a given command.
 
@@ -487,8 +479,8 @@ def spawn(
     @param returnpid: Return the Process IDs for a successful spawn.
     NOTE: This requires the caller clean up all the PIDs, otherwise spawn will clean them.
     @type returnpid: Boolean
-    @param returnproc: Return a MultiprocessingProcess instance (conflicts with logfile parameter).
-    NOTE: This requires the caller to asynchronously wait for the MultiprocessingProcess instance.
+    @param returnproc: Return a Process object for a successful spawn (conflicts with logfile parameter).
+    NOTE: This requires the caller to asynchronously wait for the Process.
     @type returnproc: Boolean
     @param uid: User ID to spawn as; useful for dropping privilages
     @type uid: Integer
@@ -631,9 +623,7 @@ def spawn(
     # fork, so that the result is cached in the main process.
     bool(groups)
 
-    start_func = _start_proc if returnproc else _start_fork
-
-    pid = start_func(
+    pid = _start_fork(
         _exec_wrapper,
         args=(
             binary,
@@ -659,10 +649,6 @@ def spawn(
         close_fds=close_fds,
     )
 
-    if returnproc:
-        # _start_proc returns a MultiprocessingProcess instance.
-        return pid
-
     if not isinstance(pid, int):
         raise AssertionError(f"fork returned non-integer: {repr(pid)}")
 
@@ -684,6 +670,8 @@ def spawn(
                 stacklevel=1,
             )
         return mypids
+    if returnproc:
+        return Process(mypids[0])
 
     # Otherwise we clean them up.
     while mypids:
@@ -1382,40 +1370,6 @@ def _start_fork(
     return pid
 
 
-def _start_proc(
-    target: Callable[..., None],
-    args: Optional[tuple[Any, ...]] = (),
-    kwargs: Optional[dict[str, Any]] = {},
-    fd_pipes: Optional[dict[int, int]] = None,
-    close_fds: Optional[bool] = False,
-) -> MultiprocessingProcess:
-    """
-    Execute the target function using multiprocess.Process.
-    If the close_fds parameter is True then NotImplementedError
-    is raised, since it is risky to forcefully close file
-    descriptors that have references (bug 374335), and PEP 446
-    should ensure that any relevant file descriptors are
-    non-inheritable and therefore automatically closed on exec.
-    """
-    if close_fds:
-        raise NotImplementedError(
-            "close_fds is not supported (since file descriptors are non-inheritable by default for exec)"
-        )
-
-    proc = ForkProcess(
-        scheduler=global_event_loop(),
-        target=target,
-        args=args,
-        kwargs=kwargs,
-        fd_pipes=fd_pipes,
-    )
-    proc.start()
-
-    # ForkProcess conveniently holds a MultiprocessingProcess
-    # instance that is suitable to return here.
-    return proc._proc
-
-
 def find_binary(binary):
     """
     Given a binary name, find the binary in PATH

diff --git a/lib/portage/tests/ebuild/test_doebuild_fd_pipes.py b/lib/portage/tests/ebuild/test_doebuild_fd_pipes.py
index c3b47d4e89..678486ed16 100644
--- a/lib/portage/tests/ebuild/test_doebuild_fd_pipes.py
+++ b/lib/portage/tests/ebuild/test_doebuild_fd_pipes.py
@@ -1,4 +1,4 @@
-# Copyright 2013-2024 Gentoo Authors
+# Copyright 2013-2023 Gentoo Authors
 # Distributed under the terms of the GNU General Public License v2
 
 import multiprocessing
@@ -167,10 +167,6 @@ class DoebuildFdPipesTestCase(TestCase):
     @staticmethod
     def _doebuild(db, pw, *args, **kwargs):
         QueryCommand._db = db
-        # Somehow pw.fileno() is inheritable when doebuild is
-        # implemented with os.fork(), but not when it is implemented
-        # with multiprocessing.Process.
-        os.set_inheritable(pw.fileno(), True)
         kwargs["fd_pipes"] = {
             DoebuildFdPipesTestCase.output_fd: pw.fileno(),
         }