gh-99368: concurrent.futures: Remove call to mp.util.debug, since mp.util is never imported

[pmitros]

trafficstars

This is a minor bug fix. The bug is documented in the associated issue: https://github.com/python/cpython/issues/99368

An alternative fix would be to import mp.util, but in this case, I don't think we want the extra debug call.

Note that I made and tested this fix on Python 3.10. I have not tested on the head of main. This is my first PR into cpython, and I'm not quite sure how your test infrastructure works, or whether this requires manual testing on main.

• Issue: gh-99368

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[cpython-cla-bot[bot]]

All commit authors signed the Contributor License Agreement.

[pmitros]

Thank you for the review and feedback.

I do not have a minimal failing test case. This is being (consistently) triggered in the context of a large system and I can replicate it. However, replacing the complex system with time.sleep causes this error to no longer occur.

You are correct about the fix, however. This code resolves the issue:

        debug = mp.util.debug
        # long commment
        def weakref_cb(_,
                       thread_wakeup=self.thread_wakeup,
                       shutdown_lock=self.shutdown_lock):
            debug('Executor collected: triggering callback for'
                          ' QueueManager wakeup')

And I've confirmed that debug is then being called. Should I make a PR like this? A closure seems cleaner than passing mp_util around as you suggested.

As a footnote, I am a little bit confused about why this code is correct. The very minimal case:

import multiprocessing as mp
mp.util.debug

does lead to an AttributeError. I'm not quite sure why this isn't happening here, but you're correct that it's not. It seems like I have something to learn once I'm past my current deadline.

[gpshead]

And I've confirmed that debug is then being called. Should I make a PR like this? A closure seems cleaner than passing mp_util around as you suggested.

I typically use the keyword arguments as a closure for this kind of destructor reference keepalive use case.

that would look like

        def weakref_cb(_,
                       thread_wakeup=self.thread_wakeup,
                       shutdown_lock=self.shutdown_lock, *,
                       _debug=mp.util.debug):
            _debug('Blah')

I don't know if that is actually practically different that your closure, but when thinking of something being called during process teardown, are the outer scope closures that weakref_cb refers to still fully populated? I don't know if that can be guaranteed or not. It's own local keyword arguments will still exist, so I know this way works.

feel free to repurpose this PR or close it and make a new one that does this.

I am a little bit confused about why this code is correct. The very minimal case:

import multiprocessing as mp
mp.util.debug

does lead to an AttributeError. I'm not quite sure why this isn't happening here, but you're correct that it's not.

This is pretty common. Nothing has imported multiprocessing.util yet. Once anything anywhere in the program does, mp.util will resolve properly.

It is correct to explicitly add an import multiprocessing.util statement to any file that later tries to reference it, even if it's gotten that import transitively for some reason in the past. It avoids this type of confusion and avoids action at a distance no longer doing the import elsewhere from impacting the behavior over here.