bpo-39678 RFC refactor queue manager thread #18551

tomMoral · 2020-02-18T15:15:08Z

As discussed in #17670 the the _queue_management_worker function has grown quite long and complicated.
This PR intends to turn it into an object with a bunch of short and readable helper methods.

I did a first pass, not sure about name changed for _queue_management_thread and a bunch of other change, in particular to cancel behavior (only do it once now). Also, I am not sure if I should add a what's new entry. Let me know if I should change anything.

https://bugs.python.org/issue39678

codecov · 2020-02-18T15:44:52Z

Codecov Report

Merging #18551 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18551      +/-   ##
==========================================
+ Coverage   82.11%   82.13%   +0.01%     
==========================================
  Files        1956     1955       -1     
  Lines      589382   584547    -4835     
  Branches    44456    44476      +20     
==========================================
- Hits       483994   480089    -3905     
+ Misses      95737    94812     -925     
+ Partials     9651     9646       -5

Impacted Files	Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py	`30.00% <0.00%> (-65.00%)`	⬇️
Lib/distutils/command/bdist_rpm.py	`7.63% <0.00%> (-56.88%)`	⬇️
Modules/_decimal/libmpdec/umodarith.h	`80.76% <0.00%> (-19.24%)`	⬇️
Lib/test/test_urllib2net.py	`76.92% <0.00%> (-13.85%)`	⬇️
Lib/test/test_smtpnet.py	`78.57% <0.00%> (-7.15%)`	⬇️
Lib/ftplib.py	`63.85% <0.00%> (-6.06%)`	⬇️
Lib/test/test_ftplib.py	`87.11% <0.00%> (-4.72%)`	⬇️
Tools/scripts/db2pickle.py	`17.82% <0.00%> (-3.97%)`	⬇️
Tools/scripts/pickle2db.py	`16.98% <0.00%> (-3.78%)`	⬇️
Lib/test/test_socket.py	`71.94% <0.00%> (-3.77%)`	⬇️
... and 345 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24bba8c...c3274e4. Read the comment docs.

tomMoral · 2020-02-18T15:46:21Z

The failure seems unrelated to this PR (in importlib).

pitrou

Thank you very much. This looks generally good, just some comments.

Lib/concurrent/futures/process.py

pitrou · 2020-02-28T18:06:53Z

Lib/concurrent/futures/process.py

-            n_children_alive = sum(p.is_alive() for p in processes.values())
+        # Set this thread to be daemonized
+        super().__init__()
+        self.daemon = True


I think it would be worthwhile exploring whether/how we can remove the daemon flag, though in another BPO issue.

From my understanding, the daemon flag is only necessary if we can't guarantee that the thread will be joined at the end of the executor's shutdown process. But, I could see this potentially being an issue when calling executor.shutdown(wait=False) for ProcessPoolExecutor (which already has issues with graceful termination).

Actually, I am not sure this is needed anymore.
The shutdown(wait=False) behavior should be fixed by #17670 and the clean up process is quite robust I believe. We should maybe give a try to simply removing the flag and experiments bad situations to see if it leads to interpreters hanging at exit.

The problem is: what happens if the interpreter starts to shutdown while an executor is still running? If the management thread is not daemon, the interpreter will hang trying to join it.

I think this is the current behavior. The _python_exit function will join the thread anyway no?

The problem is that atexit hooks are called after non-daemon threads are joined. See https://bugs.python.org/issue37266#msg362890

Ah sorry, I missed these messages.
Indeed, the simplest solution would be to call the atexit _python_exit before joining non-deamonized threads.

Lib/concurrent/futures/process.py

pitrou · 2020-02-28T18:33:18Z

@aeros You may be interested in this, at least abstractly.

aeros

Thanks for working on this @tomMoral!

After just recently working with the internals for the cancel_futures executor.shutdown() parameter, I can certainly attest to the queue management function becoming a bit convoluted. Splitting up the monolithic function into a class with multiple methods should make it significantly easier to make changes and adjustments in the future.

Lib/concurrent/futures/process.py

Misc/NEWS.d/next/Library/2020-02-28-12-59-30.bpo-39678.3idfxM.rst

Lib/concurrent/futures/process.py

tomMoral · 2020-03-01T10:57:05Z

Thanks for the reviews! I addressed your comments, let me know if there are other changes to do.

pitrou

This looks good to me now, thank you. @aeros Do you want to take another look?

aeros

LGTM, the latest commits to the PR addressed all my of concerns.

I think this PR would be a good candidate for the test-with-buildbot label, just to ensure that we don't encounter any odd side effects on other platforms prior to merging (besides the CI checks). I suspect everything will pass, but if anything doesn't, it's much simpler to address it now rather than post-commit (which could require a PR to revert the changes and another to re-apply them later).

bedevere-bot · 2020-03-01T18:29:43Z

🤖 New build scheduled with the buildbot fleet by @aeros for commit b9d5c38 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

pitrou · 2020-03-01T20:48:44Z

Buildbot failures look unrelated, I'm merging.

bpo-39678: refactor queue manager thread (pythonGH-18551)

tomMoral · 2020-03-01T21:49:13Z

thanks for the quick reviews and merge!

aeros · 2020-03-02T02:19:48Z

@pitrou In the post-commit, it looks like the failures only occurred for the Windows 7 buildbot (buildbot/AMD64 Windows7 SP1 3.x), which we no longer support since Windows 7 recently reached EoL. That buildbot has also been consistently failing across PRs recently, so it's definitely not related to the PR.

(Last time I checked, the buildbot team was working on removing all of the Windows 7 builders from the fleet, the above is the last one remaining)

So, in other words, everything looks good! Thanks again for working on the ProcessPoolExecutor refactor @tomMoral.

RFC refactor queue manager thread

1b310e7

the-knights-who-say-ni added the CLA signed label Feb 18, 2020

bedevere-bot added the awaiting review label Feb 18, 2020

tomMoral requested a review from pitrou February 18, 2020 15:15

📜🤖 Added by blurb_it.

c3274e4

pitrou reviewed Feb 28, 2020

View reviewed changes

aeros reviewed Feb 28, 2020

View reviewed changes

aeros added the type-feature A feature request or enhancement label Feb 28, 2020

tomMoral added 3 commits March 1, 2020 11:40

CLN address review comments

b30d4ed

DOC improve news entry

7af390d

CLN more comments to the references store in __init__

b9d5c38

pitrou approved these changes Mar 1, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Mar 1, 2020

aeros approved these changes Mar 1, 2020

View reviewed changes

aeros added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 1, 2020

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 1, 2020

pitrou merged commit 0e89076 into python:master Mar 1, 2020

bedevere-bot removed the awaiting merge label Mar 1, 2020

sthagen added a commit to sthagen/python-cpython that referenced this pull request Mar 1, 2020

Merge pull request #289 from python/master

1dcdec8

bpo-39678: refactor queue manager thread (pythonGH-18551)

tomMoral deleted the RFC_queue_management_thread branch March 1, 2020 21:49

tomMoral mentioned this pull request Apr 29, 2020

MTN backport some changes from cpython joblib/loky#245

Closed

Uh oh!

bpo-39678 RFC refactor queue manager thread #18551

bpo-39678 RFC refactor queue manager thread #18551

Uh oh!

Conversation

tomMoral commented Feb 18, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tomMoral commented Feb 18, 2020

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou Feb 28, 2020

Choose a reason for hiding this comment

Uh oh!

aeros Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomMoral Mar 1, 2020

Choose a reason for hiding this comment

Uh oh!

pitrou Mar 1, 2020

Choose a reason for hiding this comment

Uh oh!

tomMoral Mar 1, 2020

Choose a reason for hiding this comment

Uh oh!

pitrou Mar 1, 2020

Choose a reason for hiding this comment

Uh oh!

tomMoral Mar 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou commented Feb 28, 2020

Uh oh!

aeros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomMoral commented Mar 1, 2020

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

aeros left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Mar 1, 2020

Uh oh!

pitrou commented Mar 1, 2020

Uh oh!

tomMoral commented Mar 1, 2020

Uh oh!

aeros commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tomMoral commented Feb 18, 2020 •

edited by bedevere-bot

Loading

codecov bot commented Feb 18, 2020 •

edited

Loading

aeros Feb 28, 2020 •

edited

Loading

aeros left a comment •

edited

Loading

aeros commented Mar 2, 2020 •

edited

Loading