gh-99368: concurrent.futures: Remove call to mp.util.debug, since mp.util is never imported#99369
gh-99368: concurrent.futures: Remove call to mp.util.debug, since mp.util is never imported#99369pmitros wants to merge 1 commit intopython:mainfrom
Conversation
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
gpshead
left a comment
There was a problem hiding this comment.
while this would "fix" the issue, the mp.util call can be fixed instead, see my comment on the issue. it isn't related to being imported or not.
|
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 You are correct about the fix, however. This code resolves the issue: And I've confirmed that As a footnote, I am a little bit confused about why this code is correct. The very minimal case: does lead to an |
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.
This is pretty common. Nothing has imported It is correct to explicitly add an |


This is a minor bug fix. The bug is documented in the associated issue: #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 intocpython, and I'm not quite sure how your test infrastructure works, or whether this requires manual testing onmain.