bpo-30256: Add test for nested queues

[finefoot]

I stumbled upon this problem: https://stackoverflow.com/questions/56716470/python-multiprocessing-nested-shared-objects-doesnt-work-with-queue

Right now, Python docs state

Shared objects are capable of being nested. For example, a shared container object such as a shared list can contain other shared objects which will all be managed and synchronized by the SyncManager.

Source: https://docs.python.org/3/library/multiprocessing.html

These changes have been introduced with https://hg.python.org/cpython/rev/39e7307f9aee along with some tests. However, the tests only use nested ListProxy and DictProxy.

Because a test for AutoProxy is missing, the mentioned issue from the StackOverflow post remained undetected. This pull request adds a test for AutoProxy[Queue] for a queue nested inside a list and inside a dict.

Hence, the test added by this pull request will currently fail due to the existing bug regarding nested AutoProxy which already has a fix in #4819. After applying the patch from #4819, this test will succeed. So this pull request should be merged together with #4819.

https://bugs.python.org/issue30256

https://bugs.python.org/issue30256

[finefoot]

@brandtbucher Thanks for adding the labels. :) Do you mind adding the "skip news" label to this pull request and the bugfix one #4819, too? It's the only CI check missing from #4819 and as far as I understand, a news entry for that small patch is not necessary. The failed CI check might be the only thing missing from getting #4819 merged.

[brandtbucher]

Hi @finefoot! Thanks and welcome to CPython! 😎

I think that this PR (and certainly the other one) still need NEWS entries. "Trivial" changes and "small" changes don't always mean the same thing, and my personal litmus test is whether or not the changes touch live code. A NEWS entry only takes seconds to write, but can be extremely valuable when digging through changes or debugging regressions later. It seems, at least in my experience, that first-timers are usually a bit too hesitant to include them.

I'll take a look over at #4819 and see what the holdup is.

[finefoot]

Thanks for the response :) Sorry, you're right - I had a look at only the most recent pull request which introduced a change to _test_multiprocessing.py which doesn't have a NEWS entry and makes use of the "skip news" label. I should have also looked at a few old pull requests which all do have a NEWS entry. ;) So 3f6cd15 should fix that now, I think.

[pitrou]

Sorry for the delay. @finefoot is it possible for you to integrate the #4819 fix in this PR? It will make it easier to review and merge.

[finefoot]

@pitrou I thought, I'd just adopt the changes directly from master once #4819 got merged. (To show the tests will pass, then.) But you want me to add the changes to managers.py with an individual commit before that, yes?

[pitrou]

The fix for a bug and the tests for it should be logically contained in a single PR, so I'd like this PR (or the other one) to contain both.

[finefoot]

@pitrou 👍 Done, I've merged the changes from #4819 and the tests just passed.

[codecov]

Codecov Report

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

@@             Coverage Diff             @@
##           master   #16341       +/-   ##
===========================================
+ Coverage   79.56%   82.06%    +2.50%     
===========================================
  Files         381     1955     +1574     
  Lines      168109   584066   +415957     
  Branches        0    44458    +44458     
===========================================
+ Hits       133753   479337   +345584     
- Misses      34356    95105    +60749     
- Partials        0     9624     +9624     

Impacted Files	Coverage Δ
Modules/_decimal/libmpdec/umodarith.h	80.76% <0.00%> (-19.24%)	⬇️
Modules/fcntlmodule.c	71.17% <0.00%> (-10.26%)	⬇️
Modules/_ctypes/_ctypes_test.c	62.67% <0.00%> (-7.90%)	⬇️
Include/cpython/abstract.h	97.36% <0.00%> (-2.64%)	⬇️
Modules/clinic/mathmodule.c.h	85.71% <0.00%> (-2.33%)	⬇️
Modules/clinic/fcntlmodule.c.h	60.91% <0.00%> (-2.30%)	⬇️
Objects/codeobject.c	82.59% <0.00%> (-2.20%)	⬇️
Modules/syslogmodule.c	81.25% <0.00%> (-2.09%)	⬇️
Modules/_sqlite/util.c	64.86% <0.00%> (-1.81%)	⬇️
Modules/readline.c	73.51% <0.00%> (-1.43%)	⬇️
... and 1724 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 bfd0c96...b5877a0. Read the comment docs.

[pitrou]

@finefoot You can leave the codecov failure alone, it's not relevant. The patch is in my todo list, it's just that I need to familiarize a bit with the code around that feature.

[applio]

I first left his comment on #4819 but I perhaps should have added it here instead:

When testing this fix, this post on StackOverflow suggests they experienced a seg fault: https://stackoverflow.com/questions/56716470/multiprocessing-manager-nested-shared-objects-doesnt-work-with-queue

Has this been investigated and resolved?

[finefoot]

Note that the solution I referenced in comments in @Darkonaut's answer produced seg faults in my own testing, but this solution did not.

From https://stackoverflow.com/a/58668088

[uSpike]

Thank you @finefoot for creating this MR with additional tests. As I mentioned in #4819 and @finefoot noted above, it seems to me that the noted stackoverflow comment was not suggesting that this change causes crashes. And, two other commenters in #4819 including myself are unable to reproduce it. It'd be really, really, really nice to have this in 3.10.

[gvanrossum]

LGTM. I'm not sure how the test relates to the fix, but I'm too lazy to run all the tests and I trust this. I'll add a Co-Authored-By: uSpike header since the patch is @uSpike's.

[gvanrossum]

I'll close and reopen this PR to force the tests to re-run, otherwise I can't merge it.

[gvanrossum]

...Aaaaand we're back!

[gvanrossum]

• What I should add:

Co-Authored-By:  Jordan Speicher <[email protected]>

• Waiting for tests to pass.
• Hopefully I can merge despite the failing codecov test.

[miss-islington]

Thanks @finefoot for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9.
🐍🍒⛏🤖

[bedevere-bot]

GH-26987 is a backport of this pull request to the 3.10 branch.

[miss-islington]

Thanks @finefoot for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

[bedevere-bot]

GH-26989 is a backport of this pull request to the 3.9 branch.