bpo-38880: List interpreters associated with a channel end by LewisGaul · Pull Request #17323 · python/cpython

Thanks so much for working on this. The approach you took looks good. I've left a lot of comments, but don't get disheartened. :) I'm just thorough. I didn't see any problems with correctness. My comments mostly fall into these groups:

• use _PyInterpreterID_New()
• the Python function signature can be simpler
• simplify the code:
  • variable declarations should be where they are significant rather than at the top
  • don't worry about optimization right now
  • don't use goto where you don't need to

I'd be happy to discuss any of this further. Again, thanks for helping out!

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.

@LewisGaul, I'd like to get this wrapped up in the next week or so. What's left to be done? How can I help?

I'm looking into this right now, and would be happy to work with you to get this finished over the weekend. I'll update you after I've had a look over the next few hours.

@LewisGaul, I'd like to get this wrapped up in the next week or so. What's left to be done? How can I help?

I'm looking into this right now, and would be happy to work with you to get this finished over the weekend. I'll update you after I've had a look over the next few hours.

@ericsnowcurrently I've replaced the implementation with the suggestion you had during the review and it seems to work better, with all tests now passing except test_channel_list_interpreters_closed() which is raising ChannelClosedError when trying to list interps associated with one end when only the other end was closed. Probably just needs going through with GDB...

Awesome. Let me know if I can help in any way.

@ericsnowcurrently What seems to be happening is when I call channel_close(cid, send=True) it's going through to hit this block, which removes the channel reference entirely (on line 1078). If I've understood correctly, this call should only be closing the send end, so shouldn't be removing the channel ref?

I'd be glad to take a look with you. You open to a video chat? I'm available almost any time.

btw, I added this change in #18817 and these tests are failing. This will block my next PR as I want to open a PR on using sub-interpreters for test suite . I want to know how to help as I believe @LewisGaul you have gone a long way into finishing this.

@nanjekyejoannah I'm working on this - I have a call with Eric scheduled for Monday. The implementation is basically done, there's just an issue with one test case to do with closing a just one end of a channel. I'll let you know if any further help is required, thanks.

@ericsnowcurrently Thanks for the chat earlier.

I've updated the PR with a fix:

• In my implementation of _channel_is_associated() I call _channels_lookup()
• ChannelClosedErrors are propagated from _channels_lookup(), but this doesn't distinguish between channel ends (i.e. send end can be closed but no error is raised if recv end is still open)
• Fixed by adding a check if (send && chan->closing != NULL) { [raise ChannelClosedError] }

Should be good for re-review now!

LGTM

Thanks for working on this! I only have one comment (a possible refleak). I'll merge the PR once that is addressed.

@LewisGaul: Status check is done, and it's a failure ❌ .

@LewisGaul: Status check is done, and it's a success ✅ .

Thanks @LewisGaul for your contribution. I really needed this !