Perfect your code
With built-in code review tools, GitHub makes it easy to raise the quality bar before you ship. Join the 40 million developers who've merged over 200 million pull requests.
Sign up for free See pricing for teams and enterprisesbpo-37224: Improve test__xxsubinterpreters.DestroyTests #18058
Conversation
LGTM I left small comment about formatting, but that's it. :) |
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. |
This comment has been minimized.
This comment has been minimized.
Also, I expect this needs a backport to the 3.8 branch. Please confirm. |
This comment has been minimized.
This comment has been minimized.
Also, thanks again for working on this! |
This comment has been minimized.
This comment has been minimized.
Yep, I added the |
Co-Authored-By: Eric Snow <ericsnowcurrently@gmail.com>
This comment has been minimized.
This comment has been minimized.
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.
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. |
f03a8f8
into
python:master
This comment has been minimized.
This comment has been minimized.
miss-islington
commented
Jan 31, 2020
Thanks @aeros for the PR |
aeros commentedJan 19, 2020
•
edited by miss-islington
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