Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding a SyncManager Queue proxy to a SyncManager dict or Namespace proxy raises an exception #74442

Closed
jjdmon mannequin opened this issue May 3, 2017 · 19 comments
Closed
Labels
3.9 only security fixes 3.10 only security fixes 3.11 bug and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@jjdmon
Copy link
Mannequin

jjdmon mannequin commented May 3, 2017

BPO 30256
Nosy @gvanrossum, @mjpieters, @pitrou, @hniksic, @applio, @zhangyangyu, @miss-islington, @finefoot, @Ralithune, @iritkatriel, @Fidget-Spinner
PRs
  • bpo-30256: pass all BaseProxy arguments through AutoProxy #4819
  • bpo-30256: Add test for nested queues #16341
  • [3.10] bpo-30256: Add manager_owned keyword arg to AutoProxy (GH-16341) #26987
  • [3.9] bpo-30256: Add manager_owned keyword arg to AutoProxy (GH-16341) #26988
  • [3.9] bpo-30256: Add manager_owned keyword arg to AutoProxy (GH-16341) #26989
  • bpo-30256: Fix rst in news #26994
  • [3.9] bpo-30256: [doc] Fix formatting error in news (GH-26994) #26996
  • [3.10] bpo-30256: [doc] Fix formatting error in news (GH-26994) #26998
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-07-02.04:56:34.888>
    created_at = <Date 2017-05-03.11:53:34.846>
    labels = ['type-bug', 'library', '3.9', '3.10', '3.11']
    title = 'Adding a SyncManager Queue proxy to a SyncManager dict or Namespace proxy raises an exception'
    updated_at = <Date 2021-08-03.14:08:54.256>
    user = 'https://bugs.python.org/jjdmon'

    bugs.python.org fields:

    activity = <Date 2021-08-03.14:08:54.256>
    actor = 'kj'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-07-02.04:56:34.888>
    closer = 'gvanrossum'
    components = ['Library (Lib)']
    creation = <Date 2017-05-03.11:53:34.846>
    creator = 'jjdmon'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30256
    keywords = ['patch']
    message_count = 19.0
    messages = ['292889', '311592', '311594', '346334', '352960', '361034', '376584', '396839', '396842', '396843', '396844', '396845', '396846', '396869', '396871', '396872', '396873', '398828', '398829']
    nosy_count = 15.0
    nosy_names = ['gvanrossum', 'mjpieters', 'pitrou', 'hniksic', 'davin', 'xiang.zhang', 'jjdmon', 'Alexander Prokhorov', 'miss-islington', 'finefoot', 'Locane', 'Philipp Rehs', 'iritkatriel', 'kj', 'stephen.entropy']
    pr_nums = ['4819', '16341', '26987', '26988', '26989', '26994', '26996', '26998']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue30256'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @jjdmon
    Copy link
    Mannequin Author

    jjdmon mannequin commented May 3, 2017

    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.

    @jjdmon jjdmon mannequin added the stdlib Python modules in the Lib dir label May 3, 2017
    @hniksic
    Copy link
    Mannequin

    hniksic mannequin commented Feb 4, 2018

    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.

    @hniksic
    Copy link
    Mannequin

    hniksic mannequin commented Feb 4, 2018

    The issue is also present in Python 3.7.0b1.

    @hniksic hniksic mannequin added the 3.7 (EOL) end of life label Feb 4, 2018
    @finefoot
    Copy link
    Mannequin

    finefoot mannequin commented Jun 23, 2019

    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 #4819 doesn't get reviewed?

    @Ralithune
    Copy link
    Mannequin

    Ralithune mannequin commented Sep 22, 2019

    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".

    @PhilippRehs
    Copy link
    Mannequin

    PhilippRehs mannequin commented Jan 30, 2020

    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

    @PhilippRehs PhilippRehs mannequin added 3.8 only security fixes type-bug An unexpected behavior, bug, or error labels Jan 30, 2020
    @mjpieters
    Copy link
    Mannequin

    mjpieters mannequin commented Sep 8, 2020

    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.

    @gvanrossum
    Copy link
    Member

    I'm going to merge both PRs.

    @gvanrossum
    Copy link
    Member

    New changeset 85b9204 by finefoot in branch 'main':
    bpo-30256: Add manager_owned keyword arg to AutoProxy (GH-16341)
    85b9204

    @gvanrossum
    Copy link
    Member

    (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.

    @gvanrossum
    Copy link
    Member

    New changeset 3ec3e0f by Miss Islington (bot) in branch '3.10':
    bpo-30256: Add manager_owned keyword arg to AutoProxy (GH-16341) (bpo-26987)
    3ec3e0f

    @gvanrossum
    Copy link
    Member

    New changeset 8aa45de by Miss Islington (bot) in branch '3.9':
    bpo-30256: Add manager_owned keyword arg to AutoProxy (GH-16341) (GH-26989)
    8aa45de

    @gvanrossum
    Copy link
    Member

    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 #60545 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.)

    @gvanrossum gvanrossum added 3.9 only security fixes 3.10 only security fixes 3.11 bug and security fixes and removed 3.7 (EOL) end of life 3.8 only security fixes labels Jul 2, 2021
    @iritkatriel
    Copy link
    Member

    New changeset 2560c61 by Ken Jin in branch 'main':
    bpo-30256: [doc] Fix formatting error in news (GH-26994)
    2560c61

    @iritkatriel
    Copy link
    Member

    New changeset 91db097 by Miss Islington (bot) in branch '3.9':
    bpo-30256: [doc] Fix formatting error in news (GH-26994) (GH-26996)
    91db097

    @iritkatriel
    Copy link
    Member

    New changeset 7a2d2ed by Irit Katriel in branch '3.10':
    [3.10] bpo-30256: [doc] Fix formatting error in news (GH-26994) (GH-26998)
    7a2d2ed

    @gvanrossum
    Copy link
    Member

    Oh no! Was there a test for this that I ignored? Thanks for cleaning up
    after me.--
    --Guido (mobile)

    @stephenentropy
    Copy link
    Mannequin

    stephenentropy mannequin commented Aug 3, 2021

    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.

    @Fidget-Spinner
    Copy link
    Member

    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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes 3.10 only security fixes 3.11 bug and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants