Skip to content

gh-99352: Respect http.client.HTTPConnection.debuglevel in urllib.request.AbstractHTTPHandler #99353

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

Merged

Conversation

wheelerlaw
Copy link
Contributor

@wheelerlaw wheelerlaw commented Nov 10, 2022

Fixes #99352.

Some proposed changes to allow the AbstractHTTPHandler use the value of http.client.HTTPConnection.debuglevel if no debuglevel is passed to AbstractHTTPHandler's constructor. This has to be done in the constructor body because argument default values are evaluated at function definition evaluation time and http.client.HTTPConnection.debuglevel could be set after urllib.request is imported.

With these proposed changes, AbstractHTTPHandler and HTTPSHandler now respect both sources of debuglevel. If the value is not set in the constructor arguments, the constructor will source the value from http.client.HTTPConnection.debuglevel.

Using the global http.client.HTTPConnection.debuglevel:

wheeler@fedora:~/cpython$ python
Python 3.10.7 (main, Sep  7 2022, 00:00:00) [GCC 12.2.1 20220819 (Red Hat 12.2.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import http.client
>>> import urllib.request
>>> http.client.HTTPConnection.debuglevel=1
>>> urllib.request.urlopen("http://example.com")
send: b'GET / HTTP/1.1\r\nAccept-Encoding: identity\r\nHost: example.com\r\nUser-Agent: Python-urllib/3.10\r\nConnection: close\r\n\r\n'
reply: 'HTTP/1.1 200 OK\r\n'
header: Age: 486527
header: Cache-Control: max-age=604800
header: Content-Type: text/html; charset=UTF-8
header: Date: Thu, 10 Nov 2022 22:00:09 GMT
header: Etag: "3147526947+gzip+ident"
header: Expires: Thu, 17 Nov 2022 22:00:09 GMT
header: Last-Modified: Thu, 17 Oct 2019 07:18:26 GMT
header: Server: ECS (cha/80C2)
header: Vary: Accept-Encoding
header: X-Cache: HIT
header: Content-Length: 1256
header: Connection: close
<http.client.HTTPResponse object at 0x7f322c674a00>
>>> 

Using the debuglevel constructor parameter:

wheeler@fedora:~/cpython$ python
Python 3.10.7 (main, Sep  7 2022, 00:00:00) [GCC 12.2.1 20220819 (Red Hat 12.2.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import urllib.request
>>> handler = urllib.request.HTTPHandler(debuglevel=1)
>>> opener = urllib.request.build_opener(handler)
>>> opener.open("http://example.com")
send: b'GET / HTTP/1.1\r\nAccept-Encoding: identity\r\nHost: example.com\r\nUser-Agent: Python-urllib/3.10\r\nConnection: close\r\n\r\n'
reply: 'HTTP/1.1 200 OK\r\n'
header: Age: 567191
header: Cache-Control: max-age=604800
header: Content-Type: text/html; charset=UTF-8
header: Date: Thu, 10 Nov 2022 22:06:43 GMT
header: Etag: "3147526947+ident"
header: Expires: Thu, 17 Nov 2022 22:06:43 GMT
header: Last-Modified: Thu, 17 Oct 2019 07:18:26 GMT
header: Server: ECS (cha/8096)
header: Vary: Accept-Encoding
header: X-Cache: HIT
header: Content-Length: 1256
header: Connection: close
<http.client.HTTPResponse object at 0x7ff801e373d0>
>>> 

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@ghost
Copy link

ghost commented Nov 10, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@wheelerlaw wheelerlaw force-pushed the bugfix/respect-http-connection-debuglevel branch from 353519d to 518d0e2 Compare November 10, 2022 22:14
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@wheelerlaw wheelerlaw force-pushed the bugfix/respect-http-connection-debuglevel branch from ac4037b to cc7f8d9 Compare November 10, 2022 22:28
@wheelerlaw
Copy link
Contributor Author

Who do I need to mention to get a review on this?

Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

(be sure to pull first, i pushed an update to the news entry in the branch)

@@ -1048,7 +1057,30 @@ def test_http_body_array(self):
newreq = h.do_request_(req)
self.assertEqual(int(newreq.get_header('Content-length')),16)

def test_http_handler_debuglevel(self):
def test_http_handler_global_debuglevel(self):
http.client.HTTPConnection.debuglevel = 1
Copy link
Member

Choose a reason for hiding this comment

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

use a with mock.patch.object(http.client.HTTPConnection, 'debuglevel, 1): style block to set this so that it is undone after the test. same below. I also suggest using a value other than 1 so that it is more obviously correlated with the specific test and not a default from elsewhere.

@bedevere-bot
Copy link

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.

@gpshead gpshead self-assigned this Feb 7, 2023
wheelerlaw and others added 5 commits April 20, 2023 18:32
* Use mock.patch.object instead of settting the module level value.
* Used test values to assert the debuglevel.
@orsenthil orsenthil force-pushed the bugfix/respect-http-connection-debuglevel branch from 031e8a9 to ad096d4 Compare April 21, 2023 01:32
@orsenthil orsenthil requested a review from gpshead April 21, 2023 01:32
Copy link
Member

@orsenthil orsenthil 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 have addressed the review comments against this change.

@orsenthil orsenthil merged commit 5c00a62 into python:main Apr 21, 2023
@wheelerlaw wheelerlaw deleted the bugfix/respect-http-connection-debuglevel branch April 22, 2023 02:47
@ericsnowcurrently
Copy link
Member

This appears to have caused failures on several buildbots. For example, https://buildbot.python.org/all/#/builders/15/builds/4336.

@ericsnowcurrently
Copy link
Member

@orsenthil, this is the one I was talking about.

@orsenthil
Copy link
Member

Oh. I see the issue now. Didn't realize we were disabling ssl in some builds. I will fix it. Sorry.

@orsenthil
Copy link
Member

Here we go - #103828 ; I can test against the failing buildbot before merging.

orsenthil added a commit that referenced this pull request Apr 26, 2023
…s tests. (#103828)

gh-99352: Ensure HTTPSConnection is available before exercising https
tests.

This will fix the buildbot issue mentioned in

#99353
itamaro pushed a commit to itamaro/cpython that referenced this pull request Apr 26, 2023
…g https tests. (python#103828)

pythongh-99352: Ensure HTTPSConnection is available before exercising https
tests.

This will fix the buildbot issue mentioned in

python#99353
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

urllib.request.urlopen() no longer respects the http.client.HTTPConnection.debuglevel flag
5 participants