Skip to content

bpo-30256: pass all BaseProxy arguments through AutoProxy - #4819

Closed
uSpike wants to merge 1 commit into
python:masterfrom
uSpike:multiprocessing_autoproxy_fix
Closed

bpo-30256: pass all BaseProxy arguments through AutoProxy#4819
uSpike wants to merge 1 commit into
python:masterfrom
uSpike:multiprocessing_autoproxy_fix

Conversation

@uSpike

@uSpikeuSpike commented Dec 12, 2017

Copy link
Copy Markdown
Contributor

The AutoProxy method did not accept the "manager_owned" keyword argument which broke when nested AutoProxy objects were created.

https://bugs.python.org/issue30256

@the-knights-who-say-ni

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@uSpikeuSpike changed the title bpo30256: pass all BaseProxy arguments through AutoProxybpo-30256: pass all BaseProxy arguments through AutoProxyDec 12, 2017
@uSpike

Copy link
Copy Markdown
ContributorAuthor

I signed the CLA (over a month ago) and my bpo account reflects that, however @the-knights-who-say-ni has not updated yet.

@Phhere

Copy link
Copy Markdown

When will this get merged ?

@finefoot

finefoot commented Jun 23, 2019

Copy link
Copy Markdown
Contributor

I stumbled upon this problem, too: 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.

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.

So maybe this PR should add tests for AutoProxy[Queue], too?


Update: Some tests have been added with #16341

@Phhere

Copy link
Copy Markdown

@uSpike could you add an entry to Misc/News.d/next for this issue? maybe it get merged when CI is fine

@carlbordum

Copy link
Copy Markdown
Contributor

Hey I just want to say that this patch fixed my issue.

@brandtbucherbrandtbucher added the type-bug An unexpected behavior, bug, or error label Sep 25, 2019
@brandtbucher

Copy link
Copy Markdown
Member

@pitrou, care to review? It looks like there's at least one PR waiting on this.

I'm not too familiar with multiprocessing, but the change looks good. The only thing that I think should be added is a NEWS entry (#16341 contains a regression test from a first-timer, but it can't be merged yet).

@uSpike
uSpikeforce-pushed the multiprocessing_autoproxy_fix branch from aacc4ba to 2cc5894CompareSeptember 25, 2019 18:56
@uSpike

Copy link
Copy Markdown
ContributorAuthor

@brandtbucher I've added a NEWS entry.

@brandtbucher

Copy link
Copy Markdown
Member

Awesome, thanks for this @uSpike. And welcome to CPython!

@Phhere

Phhere commented Jan 30, 2020

Copy link
Copy Markdown

When will it be merged ?

I think @applio it not active any more on this project.
@pitrou can you review it ?

@jimenez-alex

Copy link
Copy Markdown

@pitrou | ( @Phhere ) any ETA for this simple change to be reviewed?

@tstordyallison

Copy link
Copy Markdown

Any update on this one? Feels like a very easy fix :)

@applio around for a quick approval?

@Phhere

Phhere commented Apr 28, 2020

Copy link
Copy Markdown

Maybe one of these can help:
@pitrou
@vstinner

@uSpike

Copy link
Copy Markdown
ContributorAuthor

Gentle bump? This PR has been open for 2.5 years, it's a trivial change, and I'd love to be a contributor! I really don't want to miss the window for 3.9.

@nanjekyejoannahnanjekyejoannah left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update : I was wondering it a test exists and I saw the test here : #16341 sorry.

@pablogsal
pablogsal requested a review from pitrouMay 6, 2020 15:17
@applio

Copy link
Copy Markdown
Member

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?

@Phhere

Copy link
Copy Markdown

I was not able to reproduce any seg faults with this patch

@StefanD986

Copy link
Copy Markdown

I tried this patch and I did not get any segfaults.

Would be nice if this could get fixed into python 3.9.

My system:
Python 3.7.9 (tags/v3.7.9:13c94747c7, Aug 17 2020, 18:58:18) [MSC v.1900 64 bit (AMD64)] on win32

@uSpike

Copy link
Copy Markdown
ContributorAuthor

I'm also unable to reproduce any seg faults in 3.7.5 on Ubuntu 20.04. I may be wrong, but I think the stackoverflow message that @applio is referencing is not suggesting that this patch causes a seg fault. The code he says "works" is the same as this patch.

@uSpike

Copy link
Copy Markdown
ContributorAuthor

I'm closing this in favor of #16341

@uSpikeuSpike closed this Feb 24, 2021
@uSpike
uSpike deleted the multiprocessing_autoproxy_fix branch July 2, 2021 04:03
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting core reviewtype-bugAn unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

14 participants

@uSpike@the-knights-who-say-ni@Phhere@finefoot@carlbordum@brandtbucher@jimenez-alex@tstordyallison@applio@StefanD986@nanjekyejoannah@ned-deily@Mariatta@bedevere-bot