FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-28254: _posixsubprocess uses PyGC_Enable/PyGC_Disable by vstinner · Pull Request #25693 · python/cpython · GitHub

Repository navigation

bpo-28254: _posixsubprocess uses PyGC_Enable/PyGC_Disable - #25693

Merged
vstinner merged 2 commits into
python:masterfrom
vstinner:_posixsubprocess
Apr 28, 2021
Merged

vstinner merged 2 commits into
python:masterfrom
vstinner:_posixsubprocess

Conversation

vstinner commented Apr 28, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Copy link
Copy Markdown
Member Author

cc @pablogsal @scoder

Copy link
Copy Markdown
Member
 ======================================================================
FAIL: test_preexec_gc_module_failure (test.test_subprocess.POSIXProcessTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython/Lib/test/test_subprocess.py", line 2170, in test_preexec_gc_module_failure
    self.assertRaises(RuntimeError, subprocess.Popen,
AssertionError: RuntimeError not raised by Popen

👀

Copy link
Copy Markdown
Member Author

Thanks. I fixed test_subprocess.

vstinner merged commit 103d5e4 into python:master Apr 28, 2021
vstinner deleted the _posixsubprocess branch April 28, 2021 17:09

scoder commented Apr 28, 2021

Copy link
Copy Markdown
Contributor

Actually, I think much of this could already have been saved before by using the ID-string API. But still, this is just so much better.

Copy link
Copy Markdown
Member Author

Actually, I think much of this could already have been saved before by using the ID-string API. But still, this is just so much better.

I don't think that using cached interned strings is really relevant here, spawning a subprocess takes a few milliseconds, we are far from nanoseconds avoided thanks to the cache. Anyway, it's now gone ;-)

gpshead 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yay, thanks for the cleanup!

preexec_fn=lambda: None)
finally:
gc.disable = orig_gc_disable
gc.isenabled = orig_gc_isenabled

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

saving and restoring these two with the try: finally: seems no longer necessary now that they're not being monkeypatched out to cause errors. (harmless, but anyone reading this will wonder why...)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh, you're right: I created #25709

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

6 participants


Back | FazBrowse Home | New Git URL