Skip to content

gh-156031: Restore urllib.request._opener in test_httpservers - #156032

Open
ayaangazali wants to merge 2 commits into
python:mainfrom
ayaangazali:gh-156031-httpservers-opener-leak
Open

gh-156031: Restore urllib.request._opener in test_httpservers#156032
ayaangazali wants to merge 2 commits into
python:mainfrom
ayaangazali:gh-156031-httpservers-opener-leak

Conversation

@ayaangazali

Copy link
Copy Markdown

test_httpservers leaves urllib.request._opener installed for the rest of the test run, so regrtest flags it as altering the execution environment. Details and the trace are in gh-156031.

Short version: CommandLineRunTimeTestCase.fetch_file() calls urlopen() with no SSL context for the plain HTTP cases, and urlopen() only builds a throwaway opener when a context is passed. Otherwise it assigns the module global:

elif _opener is None:
    _opener = opener = build_opener()

test_urllib already guards the same global with self.addCleanup(urllib.request.urlcleanup) in four places, and urlcleanup() resets _opener, so this uses the same pattern rather than inventing a new one.

Verifying

Repro takes about two seconds. test_urllib runs first only to get urllib.request into sys.modules, which is what makes regrtest watch the resource at all:

./python.exe -m test test_urllib test_httpservers

Before:

Warning -- urllib.requests._opener was modified by test_httpservers
test_httpservers failed (env changed)

After: SUCCESS, same 209 tests run.

Also ran the whole suite sequentially, which is the only mode that detects this, before and after:

before after
tests OK 459 460
env changed 1 (test_httpservers) 0
failed 1 (test_struct) 1 (test_struct)

test_httpservers goes from failed (env changed) to passed. The test_struct failure is unrelated and present before my change too, so I left it alone. pre-commit passes on the touched file.

Worth noting why this has not turned up in CI: save_env.py only watches the resource when urllib.request is already imported, because try_get_module() raises SkipTestEnvironment otherwise. Under -j each test file gets a fresh subprocess, so the module is not in sys.modules when regrtest snapshots the environment and the resource is never tracked. Running test_httpservers on its own is clean for the same reason. It only shows up sequentially, after something earlier has imported urllib.request.

No test added. The fix is test-only hygiene and the suite run above is the check, so a new test would just restate it.

apologies if any of this is off. I found it by running the suite sequentially, traced the cause myself, and talked the fix choice over with Claude Code before settling on matching the existing test_urllib pattern. happy to be corrected on any of it. freshman in college, doing what I can to help out :)

CommandLineRunTimeTestCase.fetch_file() calls urlopen() without an SSL
context for the plain HTTP cases. urlopen() only builds a throwaway opener
when a context is passed, so those calls install the urllib.request._opener
module global and leave it set for the rest of the test run.

test_urllib already guards the same global with addCleanup(urlcleanup) in
several places; do the same here.
Copilot AI lite review requested due to automatic review settings August 19, 2026 00:03
@bedevere-app bedevere-app Bot added the tests Tests in the Lib/test dir label Aug 19, 2026
@bedevere-app

bedevere-app Bot commented Aug 19, 2026

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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.

Test only changes don't requires a news entry file.

@aisk aisk added the skip news label Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review skip news tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants