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

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

Closed
wants to merge 1 commit into from

Conversation

@uSpike
Copy link
Contributor

@uSpike uSpike commented Dec 12, 2017

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

@the-knights-who-say-ni the-knights-who-say-ni commented Dec 12, 2017

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!

@uSpike uSpike changed the title bpo30256: pass all BaseProxy arguments through AutoProxy bpo-30256: pass all BaseProxy arguments through AutoProxy Dec 12, 2017
@uSpike
Copy link
Contributor Author

@uSpike uSpike commented Jan 22, 2018

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

@Phhere Phhere commented Mar 10, 2019

When will this get merged ?

@finefoot
Copy link
Contributor

@finefoot finefoot commented Jun 23, 2019

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

@Phhere Phhere commented Jul 19, 2019

@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
Contributor

@carlbordum carlbordum commented Aug 8, 2019

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

@brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Sep 25, 2019

@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 uSpike force-pushed the multiprocessing_autoproxy_fix branch from aacc4ba to 2cc5894 Sep 25, 2019
@uSpike
Copy link
Contributor Author

@uSpike uSpike commented Sep 25, 2019

@brandtbucher I've added a NEWS entry.

@brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Sep 25, 2019

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

@matrixise matrixise requested a review from applio Sep 26, 2019
@Phhere
Copy link

@Phhere Phhere commented Jan 30, 2020

When will it be merged ?

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

Phhere
Phhere approved these changes Jan 30, 2020
@jimenez-alex
Copy link

@jimenez-alex jimenez-alex commented Mar 4, 2020

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

@tstordyallison
Copy link

@tstordyallison tstordyallison commented Apr 22, 2020

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

@applio around for a quick approval?

@Phhere
Copy link

@Phhere Phhere commented Apr 28, 2020

Maybe one of these can help:
@pitrou
@vstinner

@uSpike
Copy link
Contributor Author

@uSpike uSpike commented May 6, 2020

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.

Copy link
Contributor

@nanjekyejoannah nanjekyejoannah left a comment

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

@pablogsal pablogsal requested a review from pitrou May 6, 2020
@applio
Copy link
Member

@applio applio commented May 6, 2020

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

@Phhere Phhere commented May 8, 2020

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

@StefanD986
Copy link

@StefanD986 StefanD986 commented Oct 5, 2020

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
Contributor Author

@uSpike uSpike commented Feb 24, 2021

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
Contributor Author

@uSpike uSpike commented Feb 24, 2021

I'm closing this in favor of #16341

@uSpike uSpike closed this Feb 24, 2021
@uSpike uSpike deleted the multiprocessing_autoproxy_fix branch Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet