◐ Shell
reader mode source ↗
Skip to content

bpo-40390: Implement channel_send_wait for subinterpreters#19715

Closed
benedwards14 wants to merge 4 commits into
python:masterfrom
benedwards14:feature/channel-send-wait
Closed

bpo-40390: Implement channel_send_wait for subinterpreters#19715
benedwards14 wants to merge 4 commits into
python:masterfrom
benedwards14:feature/channel-send-wait

Conversation

@benedwards14

@benedwards14 benedwards14 commented Apr 25, 2020

Copy link
Copy Markdown
Contributor

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Thanks for working on this! Overall the implementation seems good. I've left some comments on things to address.

Regarding the approach to providing a blocking "send()"... After having seen the block-in-the-function approach here, I still have reservations about it. There's a lot of value to keeping the "send" part separate from the "wait" part.

Here are some alternatives that offer that separation:

  • return an acquired threading.Lock which the receiving interpreter releases
  • return a wait(timeout=None) function which the receiving interpreter will un-block
  • caller passes in an acquired lock that the receiving interpreter releases
  • caller passes in a callback function that the receiving interpreter triggers (via _Py_AddPendingCall())

My preference is that first one (return a lock). Since we are crossing the interpreter boundary we need to be extra careful. A user-defined callback is too risky. Of the remaining options the first one is simplest and most consistent conceptually.

FWIW, I expect that most of the code in this PR should still work mostly as-is.

5 hidden conversations Load more…
@bedevere-bot

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@bedevere-bot bedevere-bot added the label Apr 28, 2020
@ericsnowcurrently

Copy link
Copy Markdown
Member

FYI, there is a decent chance that we will make threading.Lock shareable (via channels). So keep that in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants