#105 [DO NOT MERGE] Revert upstream commit 3b699932e5ac3e7
Closed 4 months ago by churchyard. Opened 5 months ago by churchyard.
rpms/ churchyard/python3 revert_locks_again  into  master

@@ -0,0 +1,168 @@ 

+ From 583bb07116505dcc73c0fc4aa3c1cf5074f3522d Mon Sep 17 00:00:00 2001

+ From: =?UTF-8?q?Miro=20Hron=C4=8Dok?= <miro@hroncok.cz>

+ Date: Fri, 2 Nov 2018 14:01:28 +0100

+ Subject: [PATCH] Revert "bpo-6721: Hold logging locks across fork() (GH-4071)

+  (#9291)"

+ 

+ This reverts commit 3b699932e5ac3e76031bbb6d700fbea07492641d.

+ ---

+  Lib/logging/__init__.py  | 50 ------------------------------

+  Lib/test/test_logging.py | 67 ----------------------------------------

+  2 files changed, 117 deletions(-)

+ 

+ diff --git a/Lib/logging/__init__.py b/Lib/logging/__init__.py

+ index 3ad2cc38f6..53780cd600 100644

+ --- a/Lib/logging/__init__.py

+ +++ b/Lib/logging/__init__.py

+ @@ -225,55 +225,6 @@ def _releaseLock():

+      if _lock:

+          _lock.release()

+  

+ -

+ -# Prevent a held logging lock from blocking a child from logging.

+ -

+ -if not hasattr(os, 'register_at_fork'):  # Windows and friends.

+ -    def _register_at_fork_acquire_release(instance):

+ -        pass  # no-op when os.register_at_fork does not exist.

+ -else:  # The os.register_at_fork API exists

+ -    os.register_at_fork(before=_acquireLock,

+ -                        after_in_child=_releaseLock,

+ -                        after_in_parent=_releaseLock)

+ -

+ -    # A collection of instances with acquire and release methods (logging.Handler)

+ -    # to be called before and after fork.  The weakref avoids us keeping discarded

+ -    # Handler instances alive forever in case an odd program creates and destroys

+ -    # many over its lifetime.

+ -    _at_fork_acquire_release_weakset = weakref.WeakSet()

+ -

+ -

+ -    def _register_at_fork_acquire_release(instance):

+ -        # We put the instance itself in a single WeakSet as we MUST have only

+ -        # one atomic weak ref. used by both before and after atfork calls to

+ -        # guarantee matched pairs of acquire and release calls.

+ -        _at_fork_acquire_release_weakset.add(instance)

+ -

+ -

+ -    def _at_fork_weak_calls(method_name):

+ -        for instance in _at_fork_acquire_release_weakset:

+ -            method = getattr(instance, method_name)

+ -            try:

+ -                method()

+ -            except Exception as err:

+ -                # Similar to what PyErr_WriteUnraisable does.

+ -                print("Ignoring exception from logging atfork", instance,

+ -                      method_name, "method:", err, file=sys.stderr)

+ -

+ -

+ -    def _before_at_fork_weak_calls():

+ -        _at_fork_weak_calls('acquire')

+ -

+ -

+ -    def _after_at_fork_weak_calls():

+ -        _at_fork_weak_calls('release')

+ -

+ -

+ -    os.register_at_fork(before=_before_at_fork_weak_calls,

+ -                        after_in_child=_after_at_fork_weak_calls,

+ -                        after_in_parent=_after_at_fork_weak_calls)

+ -

+ -

+  #---------------------------------------------------------------------------

+  #   The logging record

+  #---------------------------------------------------------------------------

+ @@ -844,7 +795,6 @@ class Handler(Filterer):

+          Acquire a thread lock for serializing access to the underlying I/O.

+          """

+          self.lock = threading.RLock()

+ -        _register_at_fork_acquire_release(self)

+  

+      def acquire(self):

+          """

+ diff --git a/Lib/test/test_logging.py b/Lib/test/test_logging.py

+ index 1ea2967f5b..5d5eba7357 100644

+ --- a/Lib/test/test_logging.py

+ +++ b/Lib/test/test_logging.py

+ @@ -35,7 +35,6 @@ import os

+  import queue

+  import random

+  import re

+ -import signal

+  import socket

+  import struct

+  import sys

+ @@ -667,72 +666,6 @@ class HandlerTest(BaseTest):

+                  if os.path.exists(fn):

+                      os.unlink(fn)

+  

+ -    # The implementation relies on os.register_at_fork existing, but we test

+ -    # based on os.fork existing because that is what users and this test use.

+ -    # This helps ensure that when fork exists (the important concept) that the

+ -    # register_at_fork mechanism is also present and used.

+ -    @unittest.skipIf(not hasattr(os, 'fork'), 'Test requires os.fork().')

+ -    def test_post_fork_child_no_deadlock(self):

+ -        """Ensure forked child logging locks are not held; bpo-6721."""

+ -        refed_h = logging.Handler()

+ -        refed_h.name = 'because we need at least one for this test'

+ -        self.assertGreater(len(logging._handlers), 0)

+ -

+ -        locks_held__ready_to_fork = threading.Event()

+ -        fork_happened__release_locks_and_end_thread = threading.Event()

+ -

+ -        def lock_holder_thread_fn():

+ -            logging._acquireLock()

+ -            try:

+ -                refed_h.acquire()

+ -                try:

+ -                    # Tell the main thread to do the fork.

+ -                    locks_held__ready_to_fork.set()

+ -

+ -                    # If the deadlock bug exists, the fork will happen

+ -                    # without dealing with the locks we hold, deadlocking

+ -                    # the child.

+ -

+ -                    # Wait for a successful fork or an unreasonable amount of

+ -                    # time before releasing our locks.  To avoid a timing based

+ -                    # test we'd need communication from os.fork() as to when it

+ -                    # has actually happened.  Given this is a regression test

+ -                    # for a fixed issue, potentially less reliably detecting

+ -                    # regression via timing is acceptable for simplicity.

+ -                    # The test will always take at least this long. :(

+ -                    fork_happened__release_locks_and_end_thread.wait(0.5)

+ -                finally:

+ -                    refed_h.release()

+ -            finally:

+ -                logging._releaseLock()

+ -

+ -        lock_holder_thread = threading.Thread(

+ -                target=lock_holder_thread_fn,

+ -                name='test_post_fork_child_no_deadlock lock holder')

+ -        lock_holder_thread.start()

+ -

+ -        locks_held__ready_to_fork.wait()

+ -        pid = os.fork()

+ -        if pid == 0:  # Child.

+ -            logging.error(r'Child process did not deadlock. \o/')

+ -            os._exit(0)

+ -        else:  # Parent.

+ -            fork_happened__release_locks_and_end_thread.set()

+ -            lock_holder_thread.join()

+ -            start_time = time.monotonic()

+ -            while True:

+ -                waited_pid, status = os.waitpid(pid, os.WNOHANG)

+ -                if waited_pid == pid:

+ -                    break  # child process exited.

+ -                if time.monotonic() - start_time > 7:

+ -                    break  # so long? implies child deadlock.

+ -                time.sleep(0.05)

+ -            if waited_pid != pid:

+ -                os.kill(pid, signal.SIGKILL)

+ -                waited_pid, status = os.waitpid(pid, 0)

+ -                self.fail("child process deadlocked.")

+ -            self.assertEqual(status, 0, msg="child process error")

+ -

+  

+  class BadStream(object):

+      def write(self, data):

+ -- 

+ 2.19.1

+ 

file modified
+12 -1

@@ -17,7 +17,7 @@ 

  #global prerel ...

  %global upstream_version %{general_version}%{?prerel}

  Version: %{general_version}%{?prerel:~%{prerel}}

- Release: 1%{?dist}

+ Release: 2%{?dist}

  License: Python

  

  

@@ -277,6 +277,13 @@ 

  # Upstream uses Debian-style architecture naming. Change to match Fedora.

  Patch274: 00274-fix-arch-names.patch

  

+ # 00312 #

+ # Revert "bpo-6721: Hold logging locks across fork() 3b699932e5ac3e7

+ # This is a TEMPORARY WORKAROUND for an urgent Fedora bug

+ # TODO Investigate properly and get a real fix (here or in anaconda)!

+ # See: https://bugzilla.redhat.com/show_bug.cgi?id=1644936

+ Patch312: 00312-revert-bpo-6721.patch

+ 

  # 00316 #

  # We remove the exe files from distutil's bdist_wininst

  # So we mark the command as unsupported - and the tests are skipped

@@ -574,6 +581,7 @@ 

  %patch205 -p1

  %patch251 -p1

  %patch274 -p1

+ %patch312 -p1

  %patch316 -p1

  

  

@@ -1491,6 +1499,9 @@ 

  # ======================================================

  

  %changelog

+ * Fri Apr 12 2019 Miro Hrončok <mhroncok@redhat.com> - 3.7.3-2

+ - Revert upstream commit 3b699932e5ac3e7 again for (#1691434)

+ 

  * Wed Mar 27 2019 Miro Hrončok <mhroncok@redhat.com> - 3.7.3-1

  - Update to 3.7.3

  

Metadata Update from @churchyard:
- Pull-request tagged with: WIP

5 months ago

OK, master doesn't build on f29 any more, so I've cherry-picked this into f29-revert_locks_again and here is another F29 scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=34124810

Pull-Request has been closed by churchyard

4 months ago

FYI the root issuse has been fixed upstream in 3.7 and master branches: https://bugs.python.org/issue36533#msg341768