Skip to content

bpo-37224: Improve test__xxsubinterpreters.DestroyTests #18058

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

Conversation

aeros
Copy link
Contributor

@aeros aeros commented Jan 19, 2020

Adds an additional assertion check based on a race condition for test__xxsubinterpreters.DestroyTests.test_still_running discovered in the bpo issue.

https://bugs.python.org/issue37224

Automerge-Triggered-By: @ericsnowcurrently

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

I left small comment about formatting, but that's it. :)

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

still LGTM 😄

I did leave a couple more formatting-related comments. Sorry for dragging this out. My intention is to be instructive rather than proscriptive. (If you don't find this helpful then please tell me.) So don't feel like making any of these changes is a prerequisite for merging. I'll give it a day before merging, to give you a chance to make any formatting changes you deem worth doing. 😄

@ericsnowcurrently
Copy link
Member

Also, I expect this needs a backport to the 3.8 branch. Please confirm.

@ericsnowcurrently
Copy link
Member

Also, thanks again for working on this! 😄

@aeros
Copy link
Contributor Author

aeros commented Jan 29, 2020

Also, I expect this needs a backport to the 3.8 branch. Please confirm.

Yep, I added the needs backport to 3.8 label when I first opened the PR. If there are any merge conflicts (and @miss-islington is unable to resolve them), I'll sort it out manually with cherry pick.

@aeros
Copy link
Contributor Author

aeros commented Jan 29, 2020

I did leave a couple more formatting-related comments. Sorry for dragging this out. My intention is to be instructive rather than proscriptive. (If you don't find this helpful then please tell me.)

They were definitely quite helpful! Although I have some experience contributing to standard library code changes, it's rather uncommon to encounter situations like this where multi-line arguments are necessary. But, it's great to know for future reference how to format it in a way that's PEP8 compliant and highly readable.

Also, thanks again for working on this!

No problem, thanks for the review! (:

After this is merged, I'll very likely do more investigation into the root cause of the running failure. It can a rather time consuming process to dive into the internals and experiment with different potential solutions. But I've recently found a strong interest in the different implementations of concurrency in the standard library, including asyncio, concurrent.futures, multiprocessing, and most certainly with your upcoming subinterpreters module.

Not to go off on too far off-topic of the PR, but I think CPython has a lot of room for improvement and increased relevance in the world of concurrent programming and building scalable, distributed systems. Many people still unfortunately view Python as "best suited for simplistic, single-threaded applications", despite it's success with concurrent network IO in asyncio. As a result, I'd like to invest as much time as I reasonably can in improving our core models of concurrency, so that CPython can be more competitive with (or even supersede) other ecosystems in that domain.

Thanks for all of your hard work with the subinterpreters module! I'm very excited about the possibilities it will pave the way for with it's drastically increased isolation and potential performance implications for CPU-bound operations.

@miss-islington miss-islington merged commit f03a8f8 into python:master Jan 31, 2020
@miss-islington
Copy link
Contributor

Thanks @aeros for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@bedevere-bot
Copy link

GH-18318 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 3, 2020
Adds an additional assertion check based on a race condition for `test__xxsubinterpreters.DestroyTests.test_still_running` discovered in the bpo issue.

https://bugs.python.org/issue37224
(cherry picked from commit f03a8f8)

Co-authored-by: Kyle Stanley <[email protected]>
miss-islington added a commit that referenced this pull request Feb 4, 2020
Adds an additional assertion check based on a race condition for `test__xxsubinterpreters.DestroyTests.test_still_running` discovered in the bpo issue.

https://bugs.python.org/issue37224
(cherry picked from commit f03a8f8)

Co-authored-by: Kyle Stanley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants