Skip to content

Commit c47943a

Browse files
[3.12] gh-121723: Relax constraints on queue objects for logging.handlers.QueueHandler. (GH-122154) (GH-122604)
(cherry picked from commit fb864c7)
1 parent 757bcfd commit c47943a

File tree

4 files changed

+124
-50
lines changed

4 files changed

+124
-50
lines changed

Doc/library/logging.config.rst

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -752,9 +752,12 @@ The ``queue`` and ``listener`` keys are optional.
752752

753753
If the ``queue`` key is present, the corresponding value can be one of the following:
754754

755-
* An actual instance of :class:`queue.Queue` or a subclass thereof. This is of course
756-
only possible if you are constructing or modifying the configuration dictionary in
757-
code.
755+
* An object implementing the :class:`queue.Queue` public API. For instance,
756+
this may be an actual instance of :class:`queue.Queue` or a subclass thereof,
757+
or a proxy obtained by :meth:`multiprocessing.managers.SyncManager.Queue`.
758+
759+
This is of course only possible if you are constructing or modifying
760+
the configuration dictionary in code.
758761

759762
* A string that resolves to a callable which, when called with no arguments, returns
760763
the :class:`queue.Queue` instance to use. That callable could be a

Lib/logging/config.py

Lines changed: 29 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -500,6 +500,33 @@ def as_tuple(self, value):
500500
value = tuple(value)
501501
return value
502502

503+
def _is_queue_like_object(obj):
504+
"""Check that *obj* implements the Queue API."""
505+
if isinstance(obj, queue.Queue):
506+
return True
507+
# defer importing multiprocessing as much as possible
508+
from multiprocessing.queues import Queue as MPQueue
509+
if isinstance(obj, MPQueue):
510+
return True
511+
# Depending on the multiprocessing start context, we cannot create
512+
# a multiprocessing.managers.BaseManager instance 'mm' to get the
513+
# runtime type of mm.Queue() or mm.JoinableQueue() (see gh-119819).
514+
#
515+
# Since we only need an object implementing the Queue API, we only
516+
# do a protocol check, but we do not use typing.runtime_checkable()
517+
# and typing.Protocol to reduce import time (see gh-121723).
518+
#
519+
# Ideally, we would have wanted to simply use strict type checking
520+
# instead of a protocol-based type checking since the latter does
521+
# not check the method signatures.
522+
queue_interface = [
523+
'empty', 'full', 'get', 'get_nowait',
524+
'put', 'put_nowait', 'join', 'qsize',
525+
'task_done',
526+
]
527+
return all(callable(getattr(obj, method, None))
528+
for method in queue_interface)
529+
503530
class DictConfigurator(BaseConfigurator):
504531
"""
505532
Configure logging using a dictionary-like object to describe the
@@ -798,32 +825,8 @@ def configure_handler(self, config):
798825
if '()' not in qspec:
799826
raise TypeError('Invalid queue specifier %r' % qspec)
800827
config['queue'] = self.configure_custom(dict(qspec))
801-
else:
802-
from multiprocessing.queues import Queue as MPQueue
803-
804-
if not isinstance(qspec, (queue.Queue, MPQueue)):
805-
# Safely check if 'qspec' is an instance of Manager.Queue
806-
# / Manager.JoinableQueue
807-
808-
from multiprocessing import Manager as MM
809-
from multiprocessing.managers import BaseProxy
810-
811-
# if it's not an instance of BaseProxy, it also can't be
812-
# an instance of Manager.Queue / Manager.JoinableQueue
813-
if isinstance(qspec, BaseProxy):
814-
# Sometimes manager or queue creation might fail
815-
# (e.g. see issue gh-120868). In that case, any
816-
# exception during the creation of these queues will
817-
# propagate up to the caller and be wrapped in a
818-
# `ValueError`, whose cause will indicate the details of
819-
# the failure.
820-
mm = MM()
821-
proxy_queue = mm.Queue()
822-
proxy_joinable_queue = mm.JoinableQueue()
823-
if not isinstance(qspec, (type(proxy_queue), type(proxy_joinable_queue))):
824-
raise TypeError('Invalid queue specifier %r' % qspec)
825-
else:
826-
raise TypeError('Invalid queue specifier %r' % qspec)
828+
elif not _is_queue_like_object(qspec):
829+
raise TypeError('Invalid queue specifier %r' % qspec)
827830

828831
if 'listener' in config:
829832
lspec = config['listener']

Lib/test/test_logging.py

Lines changed: 86 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2382,6 +2382,26 @@ class CustomListener(logging.handlers.QueueListener):
23822382
class CustomQueue(queue.Queue):
23832383
pass
23842384

2385+
class CustomQueueProtocol:
2386+
def __init__(self, maxsize=0):
2387+
self.queue = queue.Queue(maxsize)
2388+
2389+
def __getattr__(self, attribute):
2390+
queue = object.__getattribute__(self, 'queue')
2391+
return getattr(queue, attribute)
2392+
2393+
class CustomQueueFakeProtocol(CustomQueueProtocol):
2394+
# An object implementing the Queue API (incorrect signatures).
2395+
# The object will be considered a valid queue class since we
2396+
# do not check the signatures (only callability of methods)
2397+
# but will NOT be usable in production since a TypeError will
2398+
# be raised due to a missing argument.
2399+
def empty(self, x):
2400+
pass
2401+
2402+
class CustomQueueWrongProtocol(CustomQueueProtocol):
2403+
empty = None
2404+
23852405
def queueMaker():
23862406
return queue.Queue()
23872407

@@ -3869,18 +3889,16 @@ def do_queuehandler_configuration(self, qspec, lspec):
38693889
@threading_helper.requires_working_threading()
38703890
@support.requires_subprocess()
38713891
def test_config_queue_handler(self):
3872-
q = CustomQueue()
3873-
dq = {
3874-
'()': __name__ + '.CustomQueue',
3875-
'maxsize': 10
3876-
}
3892+
qs = [CustomQueue(), CustomQueueProtocol()]
3893+
dqs = [{'()': f'{__name__}.{cls}', 'maxsize': 10}
3894+
for cls in ['CustomQueue', 'CustomQueueProtocol']]
38773895
dl = {
38783896
'()': __name__ + '.listenerMaker',
38793897
'arg1': None,
38803898
'arg2': None,
38813899
'respect_handler_level': True
38823900
}
3883-
qvalues = (None, __name__ + '.queueMaker', __name__ + '.CustomQueue', dq, q)
3901+
qvalues = (None, __name__ + '.queueMaker', __name__ + '.CustomQueue', *dqs, *qs)
38843902
lvalues = (None, __name__ + '.CustomListener', dl, CustomListener)
38853903
for qspec, lspec in itertools.product(qvalues, lvalues):
38863904
self.do_queuehandler_configuration(qspec, lspec)
@@ -3900,15 +3918,21 @@ def test_config_queue_handler(self):
39003918
@support.requires_subprocess()
39013919
@patch("multiprocessing.Manager")
39023920
def test_config_queue_handler_does_not_create_multiprocessing_manager(self, manager):
3903-
# gh-120868
3921+
# gh-120868, gh-121723
39043922

39053923
from multiprocessing import Queue as MQ
39063924

39073925
q1 = {"()": "queue.Queue", "maxsize": -1}
39083926
q2 = MQ()
39093927
q3 = queue.Queue()
3910-
3911-
for qspec in (q1, q2, q3):
3928+
# CustomQueueFakeProtocol passes the checks but will not be usable
3929+
# since the signatures are incompatible. Checking the Queue API
3930+
# without testing the type of the actual queue is a trade-off
3931+
# between usability and the work we need to do in order to safely
3932+
# check that the queue object correctly implements the API.
3933+
q4 = CustomQueueFakeProtocol()
3934+
3935+
for qspec in (q1, q2, q3, q4):
39123936
self.apply_config(
39133937
{
39143938
"version": 1,
@@ -3924,21 +3948,62 @@ def test_config_queue_handler_does_not_create_multiprocessing_manager(self, mana
39243948

39253949
@patch("multiprocessing.Manager")
39263950
def test_config_queue_handler_invalid_config_does_not_create_multiprocessing_manager(self, manager):
3927-
# gh-120868
3951+
# gh-120868, gh-121723
39283952

3929-
with self.assertRaises(ValueError):
3930-
self.apply_config(
3931-
{
3932-
"version": 1,
3933-
"handlers": {
3934-
"queue_listener": {
3935-
"class": "logging.handlers.QueueHandler",
3936-
"queue": object(),
3953+
for qspec in [object(), CustomQueueWrongProtocol()]:
3954+
with self.assertRaises(ValueError):
3955+
self.apply_config(
3956+
{
3957+
"version": 1,
3958+
"handlers": {
3959+
"queue_listener": {
3960+
"class": "logging.handlers.QueueHandler",
3961+
"queue": qspec,
3962+
},
39373963
},
3938-
},
3964+
}
3965+
)
3966+
manager.assert_not_called()
3967+
3968+
@skip_if_tsan_fork
3969+
@support.requires_subprocess()
3970+
@unittest.skipUnless(support.Py_DEBUG, "requires a debug build for testing"
3971+
"assertions in multiprocessing")
3972+
def test_config_queue_handler_multiprocessing_context(self):
3973+
# regression test for gh-121723
3974+
if support.MS_WINDOWS:
3975+
start_methods = ['spawn']
3976+
else:
3977+
start_methods = ['spawn', 'fork', 'forkserver']
3978+
for start_method in start_methods:
3979+
with self.subTest(start_method=start_method):
3980+
ctx = multiprocessing.get_context(start_method)
3981+
with ctx.Manager() as manager:
3982+
q = manager.Queue()
3983+
records = []
3984+
# use 1 process and 1 task per child to put 1 record
3985+
with ctx.Pool(1, initializer=self._mpinit_issue121723,
3986+
initargs=(q, "text"), maxtasksperchild=1):
3987+
records.append(q.get(timeout=60))
3988+
self.assertTrue(q.empty())
3989+
self.assertEqual(len(records), 1)
3990+
3991+
@staticmethod
3992+
def _mpinit_issue121723(qspec, message_to_log):
3993+
# static method for pickling support
3994+
logging.config.dictConfig({
3995+
'version': 1,
3996+
'disable_existing_loggers': True,
3997+
'handlers': {
3998+
'log_to_parent': {
3999+
'class': 'logging.handlers.QueueHandler',
4000+
'queue': qspec
39394001
}
3940-
)
3941-
manager.assert_not_called()
4002+
},
4003+
'root': {'handlers': ['log_to_parent'], 'level': 'DEBUG'}
4004+
})
4005+
# log a message (this creates a record put in the queue)
4006+
logging.getLogger().info(message_to_log)
39424007

39434008
@support.requires_subprocess()
39444009
def test_multiprocessing_queues(self):
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Make :func:`logging.config.dictConfig` accept any object implementing the
2+
Queue public API. See the :ref:`queue configuration <configure-queue>`
3+
section for details. Patch by Bénédikt Tran.

0 commit comments

Comments
 (0)