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

Add Python::with_pool as a safer alternative to Python::new_pool. #3263

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

adamreichold
Copy link
Member

I am still somewhat sceptical about adding this as an unsafe API on stable. Both because the unsafe of calling with_pool "infects" the closure body which is now necessarily unsafe as well and because I see the argument applying to allow_threads which would need to be unsafe stable for the same reasons.

Personally, I think this somewhat diminishes gains made from switching to the more limited API and would hence suspect users sticking to the existing API even though it is much more prone to misuse, i.e. at the end of day I expect more programmer mistakes than if we made the API safe and ate the soundness hole just like we do for allow_threads.

@davidhewitt
Copy link
Member

davidhewitt commented Jun 21, 2023

Both because the unsafe of calling with_pool "infects" the closure body which is now necessarily unsafe as well and because I see the argument applying to allow_threads which would need to be unsafe stable for the same reasons.

I admit I'd missed the power of both of these points previously, particularly the insight on the tainting of the closure.

Given we're intending to remove this again in 0.21 which reduces my unsease of expanding API surface with a footgun, I think I should relax my position. Let's just make it safe on stable too. 👍

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

👍

@davidhewitt davidhewitt added this pull request to the merge queue Jun 21, 2023
Merged via the queue into main with commit a6e1051 Jun 21, 2023
@davidhewitt davidhewitt deleted the with-pool branch June 21, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants