Skip to content

gh-119819: Fix regression to allow logging configuration with multipr…#120030

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.

Sign up for GitHub

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

Merged
vsajip merged 2 commits into from
Jun 4, 2024
Merged

gh-119819: Fix regression to allow logging configuration with multipr… #120030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6125ac2
gh-119819: Fix regression to allow logging configuration with multipr…
vsajip Jun 4, 2024
cf2e6ed
Skip test for WASI.
vsajip Jun 4, 2024
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
4 changes: 3 additions & 1 deletion Lib/logging/config.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -781,8 +781,10 @@ def configure_handler(self, config):
# raise ValueError('No handlers specified for a QueueHandler')
if 'queue' in config:
from multiprocessing.queues import Queue as MPQueue
from multiprocessing import Manager as MM
proxy_queue = MM().Queue()
qspec = config['queue']
if not isinstance(qspec, (queue.Queue, MPQueue)):
if not isinstance(qspec, (queue.Queue, MPQueue, type(proxy_queue))):
if isinstance(qspec, str):
q = self.resolve(qspec)
if not callable(q):
Expand Down
26 changes: 26 additions & 0 deletions Lib/test/test_logging.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -3926,6 +3926,32 @@ def test_config_queue_handler(self):
msg = str(ctx.exception)
self.assertEqual(msg, "Unable to configure handler 'ah'")

@unittest.skipIf(support.is_wasi, "WASI does not have multiprocessing.")
Copy link
Member

@mhsmithmhsmithJun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of checking specifically for WASI, tests that use subprocesses should use @support.requires_subprocess().

Copy link
MemberAuthor

@vsajipvsajipJun 4, 2024•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this test doesn't use subprocess, only multiprocessing - I'm not sure if that requires_subprocess is appropriate. I can't (yet) tell whether the problem is inherent to the platform (iOS ARM64 Simulator) or, if I need to tighten the skip conditions for the test, exactly what the condition should be. It doesn't appear, from the tracebacks, that multiprocessing isn't supported - on WASI, for example, you get an error because _multiprocessing can't be imported. I'll do some more digging.

Copy link
Member

@mhsmithmhsmithJun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error on iOS is exactly the same: see the bottom of the traceback above. The fact that multiprocessing isn't available on iOS is already mentioned in the documentation.

An even more specific check would be support.skip_if_broken_multiprocessing_synchronize(), which is already used a few times in test_logging.

Copy link
MemberAuthor

@vsajipvsajipJun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error on iOS is exactly the same

Yes, I missed that for some reason. Perhaps the answer is to add a support.skip_if_no_multiprocessing, as support.skip_if_broken_multiprocessing_synchronize seems too specific for this particular test.

Copy link
Member

@mhsmithmhsmithJun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than adding a specific skip function, you could just do import_helper.import_module('_multiprocessing').

Copy link
MemberAuthor

@vsajipvsajipJun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've already done that 😄

def test_multiprocessing_queues(self):
# See gh-119819
cd = copy.deepcopy(self.config_queue_handler)
from multiprocessing import Queue as MQ, Manager as MM
q1 = MQ() # this can't be pickled
q2 = MM().Queue() # a proxy queue for use when pickling is needed
for qspec in (q1, q2):
fn = make_temp_file('.log', 'test_logging-cmpqh-')
cd['handlers']['h1']['filename'] = fn
cd['handlers']['ah']['queue'] = qspec
qh = None
try:
self.apply_config(cd)
qh = logging.getHandlerByName('ah')
self.assertEqual(sorted(logging.getHandlerNames()), ['ah', 'h1'])
self.assertIsNotNone(qh.listener)
self.assertIs(qh.queue, qspec)
self.assertIs(qh.listener.queue, qspec)
finally:
h = logging.getHandlerByName('h1')
if h:
self.addCleanup(closeFileHandler, h, fn)
else:
self.addCleanup(os.remove, fn)

def test_90195(self):
# See gh-90195
config ={
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2024-06-04-12-23-01.gh-issue-119819.WKKrYh.rst
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Fix regression to allow logging configuration with multiprocessing queue
types.