classification
Title: Adding a SyncManager Queue proxy to a SyncManager dict or Namespace proxy raises an exception
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.11, Python 3.10, Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Alexander Prokhorov, Locane, Philipp Rehs, davin, finefoot, gvanrossum, hniksic, iritkatriel, jjdmon, kj, miss-islington, mjpieters, pitrou, stephen.entropy, xiang.zhang
Priority: normal Keywords: patch

Created on 2017-05-03 11:53 by jjdmon, last changed 2021-08-03 14:08 by kj. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 4819 closed uspike, 2017-12-12 20:02
PR 16341 merged finefoot, 2019-09-24 00:52
PR 26987 merged miss-islington, 2021-07-02 03:45
PR 26988 closed miss-islington, 2021-07-02 03:45
PR 26989 merged miss-islington, 2021-07-02 04:20
PR 26994 merged kj, 2021-07-02 15:27
PR 26996 merged miss-islington, 2021-07-02 16:53
PR 26998 merged iritkatriel, 2021-07-02 17:37
Messages (19)
msg292889 - (view) Author: Jeff DuMonthier (jjdmon) Date: 2017-05-03 11:53
In multiprocessing, attempting to add a Queue proxy to a dict or Namespace proxy (all returned by the same SyncManager) raises an exception indicating a keyword argument 'manager_owned=True' has been passed to the function AutoProxy() but is not an argument of that function.

In lib/python3.6/multiprocessing/managers.py, in function RebuildProxy(), line 873: "kwds['manager_owned'] = True" adds this argument to a keyword argument dictionary.  This function calls AutoProxy which has an argument list defined on lines 909-910 as:
def AutoProxy(token, serializer, manager=None, authkey=None,
              exposed=None, incref=True):
This raises an exception because there is no manager_owned argument defined.  I added "manager_owned=False" as a keyword argument to AutoProxy which seems to have fixed the problem.  There is no exception and I am able to pass Queue proxies through dict and Namespace proxies to other processes and use them.  I don't know the purpose of that argument though or if the AutoProxy function should actually use it for something.  My fix allows but ignores it.
msg311592 - (view) Author: Hrvoje Nikšić (hniksic) * Date: 2018-02-04 08:40
I encountered this bug while testing the code in this StackOverflow answer:

    https://stackoverflow.com/a/48565011/1600898

The code at the end of the answer runs on Python 3.5, but fails on 3.6 with the "unexpected keyword argument 'manager_owned'" error.

If someone knows of a workaround until the PR is accepted, it would be appreciated as well.
msg311594 - (view) Author: Hrvoje Nikšić (hniksic) * Date: 2018-02-04 10:16
The issue is also present in Python 3.7.0b1.
msg346334 - (view) Author: (finefoot) * Date: 2019-06-23 19:10
This is still an issue: https://stackoverflow.com/questions/56716470/python-multiprocessing-nested-shared-objects-doesnt-work-with-queue

Is there a specific reason, why https://github.com/python/cpython/pull/4819 doesn't get reviewed?
msg352960 - (view) Author: Michael Tippie (Locane) Date: 2019-09-22 05:50
I am getting this error now, too.  I'm not sure what's causing it - when I use multiprocessing with a Manager.Queue, if I am passing around an object on the queue stack, I get this error.

Would really like it to "just work".
msg361034 - (view) Author: Philipp Rehs (Philipp Rehs) Date: 2020-01-30 10:34
Are there any reasons why it does not get merged?
This issue is open since more than two years and the fix is quiet small
msg376584 - (view) Author: Martijn Pieters (mjpieters) * Date: 2020-09-08 17:12
Might it be better to just *drop* the AutoProxy object altogether?

All that it adds is a delayed call to MakeProxyType(f"AutoProxy[{typeid}]", exposed) (with exposed defaulting to public_methods(instance)), per instance per process.

It could be replaced by a direct call to `MakeProxyType()`, using `public_methods` directly on the registered type. This wouldn't work for callables that are not classes or where instances add functions to the instance dict, but for those rare cases you can pass in the `exposed` argument.

The advantage is that it would simplify the codebase; no more need to special-case the BaseProxy.__reduce__ method, removing the get_methods() method on the Server class, etc. Less surface for this class of bugs to happen in the future.
msg396839 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-07-02 02:12
I'm going to merge both PRs.
msg396842 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-07-02 03:45
New changeset 85b920498b42c69185540ecc2f5c4907fd38d877 by finefoot in branch 'main':
bpo-30256: Add manager_owned keyword arg to AutoProxy (GH-16341)
https://github.com/python/cpython/commit/85b920498b42c69185540ecc2f5c4907fd38d877
msg396843 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-07-02 03:46
(The original PR was too stale to merge, so I just merged the combined PR and added a Co-Authored-By header.)

Now doing the backports.
msg396844 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-07-02 04:15
New changeset 3ec3e0f83c34eda1ad89b731e68f4a22a5f39333 by Miss Islington (bot) in branch '3.10':
bpo-30256: Add manager_owned keyword arg to AutoProxy (GH-16341) (#26987)
https://github.com/python/cpython/commit/3ec3e0f83c34eda1ad89b731e68f4a22a5f39333
msg396845 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-07-02 04:35
New changeset 8aa45de6c6d84397b772bad7e032744010bbd456 by Miss Islington (bot) in branch '3.9':
bpo-30256: Add manager_owned keyword arg to AutoProxy (GH-16341) (GH-26989)
https://github.com/python/cpython/commit/8aa45de6c6d84397b772bad7e032744010bbd456
msg396846 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-07-02 04:56
Merged and backported to 3.10, 3.9. Let's forget about 3.8 or earlier (Lukasz removed the needs-backport-to-3.8 and -3.7 labels from GH-16341 on May 4).

I should note that landing this was not a great experience:

- There's a closed PR with the fix and another PR with a confusing title that has the fix plus a test.

- The discussion on the issue and on the PRs was confusing. There was mention of a segfault reported on StackOverflow (but later it seemed to be a false alarm about a different fix), and in this issue Martijn Pieters brings up the question of whether AutoProxy is even needed, which is quite irrelevant to the bugfix.

- It's unclear from reading the code in the PR how the test relates to the fix.

- Therefore I tried to verify that the test actually failed if I undid the fix. But I found that it's rather unclear how to run the multiprocessing tests (and just those). And once I figured it out, they take a *long* time to run (there's a bug about that too somewhere).

- I tried to revive the original PR (to give credit to its author) but found that it can't be reopened because of the master->main branch rename.

- The open PR had some failing required tests so I couldn't land it without closing and reopening. More waiting. Eventually the tests passed and I could merge. On to the backports.

- One of the backport PRs randomly didn't seem to get some test statuses, so I tried to restart them by closing and reopening the PR -- only to find that miss islington deleted the branch as soon as I closed the PR, so I couldn't reopen it either. So I had to delete and re-add the "needs-backport-to-3.9" label to the main PR, and wait.

- Oh, and somehow there's a codecov test that always fails (in one of the original PRs, Antoine says to ignore it), so the backports didn't get auto-merged, so I had to wait for the other tests to pass and manually merge.

The one positive experience was Martijn Pieters' long explanation on StackOverflow of what went wrong and why this was the correct fix. (Maybe he elaborated a bit much on the monkeypatch. :-) This treatise gave me the confidence that the fix was correct (enough) and should be merged.

Parting shot: IMO we should not have accepted multiprocessing into the stdlib. It is a very useful module, but very complex, and would have been better off as a third-party module, with a more focused crew of maintainers and a quicker release cycle. (Almost everyone who uses multiprocessing needs to install other packages anyway.)
msg396869 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2021-07-02 16:53
New changeset 2560c612c89ea2534b90a266aabf76dc74d93a12 by Ken Jin in branch 'main':
bpo-30256: [doc] Fix formatting error in news (GH-26994)
https://github.com/python/cpython/commit/2560c612c89ea2534b90a266aabf76dc74d93a12
msg396871 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2021-07-02 17:38
New changeset 91db097358bcb00832e53d410035a8b7fcfdd9c3 by Miss Islington (bot) in branch '3.9':
bpo-30256: [doc] Fix formatting error in news (GH-26994) (GH-26996)
https://github.com/python/cpython/commit/91db097358bcb00832e53d410035a8b7fcfdd9c3
msg396872 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2021-07-02 17:40
New changeset 7a2d2ed1330e464ac186c09501ef51b8261f4292 by Irit Katriel in branch '3.10':
[3.10] bpo-30256: [doc] Fix formatting error in news (GH-26994) (GH-26998)
https://github.com/python/cpython/commit/7a2d2ed1330e464ac186c09501ef51b8261f4292
msg396873 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-07-02 18:57
Oh no! Was there a test for this that I ignored? Thanks for cleaning up
after me.--
--Guido (mobile)
msg398828 - (view) Author: Stephen Carboni (stephen.entropy) Date: 2021-08-03 14:03
Just chiming in to say that this is still broken for me on Python 3.9.6, Win10/64: https://pastebin.com/64F2iKaj

But, works for 3.10.0b4.
msg398829 - (view) Author: Ken Jin (kj) * (Python committer) Date: 2021-08-03 14:08
> Just chiming in to say that this is still broken for me on Python 3.9.6

From what I understand, the patch landed on 2021-07-02, 4 days after Python 3.9.6's release date of 2021-06-28, so it wasn't included.

It should come in 3.9.7, which should be out on 2021-08-30 according to PEP 596.
History
Date User Action Args
2021-08-03 14:08:54kjsetmessages: + msg398829
2021-08-03 14:03:34stephen.entropysetnosy: + stephen.entropy
messages: + msg398828
2021-07-02 18:57:39gvanrossumsetmessages: + msg396873
2021-07-02 17:40:00iritkatrielsetmessages: + msg396872
2021-07-02 17:38:13iritkatrielsetmessages: + msg396871
2021-07-02 17:37:33iritkatrielsetpull_requests: + pull_request25558
2021-07-02 16:53:34iritkatrielsetnosy: + iritkatriel
messages: + msg396869
2021-07-02 16:53:24miss-islingtonsetpull_requests: + pull_request25556
2021-07-02 15:27:09kjsetnosy: + kj

pull_requests: + pull_request25555
2021-07-02 04:56:34gvanrossumsetstatus: open -> closed

stage: patch review -> resolved
messages: + msg396846
versions: + Python 3.9, Python 3.10, Python 3.11, - Python 3.6, Python 3.7, Python 3.8
2021-07-02 04:35:46gvanrossumsetmessages: + msg396845
2021-07-02 04:20:46miss-islingtonsetpull_requests: + pull_request25551
2021-07-02 04:15:56gvanrossumsetmessages: + msg396844
2021-07-02 03:46:45gvanrossumsetresolution: fixed
messages: + msg396843
2021-07-02 03:45:33gvanrossumsetmessages: + msg396842
2021-07-02 03:45:28miss-islingtonsetpull_requests: + pull_request25550
2021-07-02 03:45:22miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request25549
2021-07-02 02:12:42gvanrossumsetnosy: + gvanrossum
messages: + msg396839
2020-09-08 17:12:13mjpieterssetnosy: + mjpieters
messages: + msg376584
2020-02-03 15:16:22Philipp Rehssettype: behavior
2020-01-30 10:34:01Philipp Rehssetnosy: + Philipp Rehs

messages: + msg361034
versions: + Python 3.8
2019-09-24 00:52:11finefootsetpull_requests: + pull_request15918
2019-09-22 05:50:49Locanesetnosy: + Locane
messages: + msg352960
2019-06-23 19:10:54finefootsetnosy: + finefoot
messages: + msg346334
2018-07-02 12:02:53xiang.zhangsetnosy: + pitrou, davin, xiang.zhang
2018-02-04 10:16:59hniksicsetmessages: + msg311594
versions: + Python 3.7
2018-02-04 08:40:20hniksicsetnosy: + hniksic
messages: + msg311592
2017-12-12 20:02:38uspikesetkeywords: + patch
stage: patch review
pull_requests: + pull_request4709
2017-11-27 22:11:39Alexander Prokhorovsetnosy: + Alexander Prokhorov
2017-05-03 11:53:34jjdmoncreate