linkcheck tests: test webservers: enable HTTP/1.1 protocol

[jayaddison]

Feature or Bugfix

• Refactoring

Purpose

• In preparation for a fix for linkcheck builder: begin using requests.Session functionality during linkchecking #11324 (use of session-based HTTP requests from the linkcheck builder), this change upgrades the communication protocol of the test webservers, allowing multiple requests to occur within a single HTTP connection.

Detail

• To ensure that the single-threaded test webserver does not become blocked by the linkcheck client, this change includes the resource-cleanup (connection close) behaviour from linkcheck builder: close streamed HTTP response objects when no content reads are required #11318.
  • edit: outdated context; multi-threaded webserving has been enabled

Relates

• Prepares for linkcheck builder: begin using requests.Session functionality during linkchecking #11324.
• Builds upon linkcheck builder: close streamed HTTP response objects when no content reads are required #11318.

[jayaddison]

I'm having difficulty replicating the unit test failures here (mostly timeouts) on Python 3.11.2.

To try to reduce the differences between my local env and the GitHub workflow, I've used python -m pytest -vv --durations 25, and have graphviz installed.

[jayaddison]

After installing a 2.x version of urllib3 (not yet supported by requests, but installed during the tests), I'm able to replicate the timeout failures that occur during these tests.

Given the unsupported-mix between requests 2.x and urllib3 2.x, perhaps it would make sense to pin urllib3<2.0.0.

[jayaddison]

Note for reviewers: without threaded webservers, HTTP1.1's ability to leave connections open causes difficulty for the Sphinx unit tests in combination with urllib3 commit urllib3/urllib3@a80c248 - a connection-pool thread safety fix, included from v2.0.0 of that library onwards.

[AA-Turner]

Looks like timeout errors on CI:

test_auth_header_uses_first_match

>       assert f"Authorization: Basic {encoded_auth}\n" in stdout
E       AssertionError: assert 'Authorization: Basic dXNlcjE6cGFzc3dvcmQ=\n' in ''

(           index: line    1) broken    http://localhost:7777/ - HTTPConnectionPool(host='localhost', port=7777): Read timed out. (read timeout=0.05)

[jayaddison]

Can you link to an example build log? That test is definitely troublesome, I'm not certain I've figured out exactly why it fails though (so any more context could be useful).

[jayaddison]

After thinking about the previous few comments (one, two, three) (apologies for the stream-of-consciousness) related to test_auth_header_uses_first_match, I've written up #11426 as a possible changeset.

The test failure in test_too_many_requests_user_timeout remains a puzzle to me; I hadn't seen that before. Perhaps it was exposed when 0817ade was added? Or is it unrelated?

Increasing the timeouts again (as in d8f15c7) could work around it short-term - but if possible I'd like to spend time figuring out whether there's a deeper problem and solution, because if there is, then I think the time spent investigating should be recovered during the application's usage on aggregate.

[jayaddison]

Nope, I'm stuck re: test_too_many_requests_user_timeout.

I'm going to remove the daemon_threads modification, moving this branch back to 94644fd, and will run the tests a few more times from there to see if the failure (or similar ones) continue to occur.

[jayaddison]

Looks like timeout errors on CI:

test_auth_header_uses_first_match

>       assert f"Authorization: Basic {encoded_auth}\n" in stdout
E       AssertionError: assert 'Authorization: Basic dXNlcjE6cGFzc3dvcmQ=\n' in ''

(           index: line    1) broken    http://localhost:7777/ - HTTPConnectionPool(host='localhost', port=7777): Read timed out. (read timeout=0.05)

Taking a break from this until next week. So far from jayaddison#3 I'm hopeful that #11426 resolves the test flakiness observed here, but also note that there was at least one unrelated timeout that occurred.

[jayaddison]

Taking a break from this until next week. So far from jayaddison#3 I'm hopeful that #11426 resolves the test flakiness observed here, but also note that there was at least one unrelated timeout that occurred.

An update: I do think that the refactor from #11426 eliminates the test_auth_header_uses_first_match test flakiness.

[jayaddison]

@AA-Turner @francoisfreitag I'd like to apologise for my extremely verbose communication while developing a bunch of the changes offered for Sphinx. I think that was quite verbose and distracting.

When you have time, could you let me know whether the pull requests I currently have open are worth progressing, and whether further changes by me would be considered? If not, that's OK - I'd prefer not to consume your and my own time if further changes from me are unlikely to be accepted.

Thanks you in advance - and I'll try to improve my communication style and approach; I'd be glad for any feedback. You've both been supportive collaboratively and helpful technically in terms of finding problems in the pull requests so far.

[francoisfreitag]

HTTP/1.1 is probably used more often than 1.0, so might as well use that in tests. 👍

[jayaddison]

Thanks again @francoisfreitag 👍

My perspective is that HTTP 1.1 is a more widely-used protocol than HTTP 1.0 nowadays, although I fully admit that that could be biased - and I didn't find statistics comparing those two protocol versions on the HTTP Archive website when I looked a few moments ago.

In some ways HTTP 1.0 seems to me to be more robust against unusual failure modes than HTTP 1.1 -- but I think that the motivating factors for it - namely more efficient use of network resources, particlarly for websites that receive high volumes of traffic - were reasonable.

[AA-Turner]

@jayaddison please could you resolve conflicts from #11426?

A

[jayaddison]

@jayaddison please could you resolve conflicts from #11426?

Done; it looks like there may be one or two more Content-Length response headers that I missed during the conflict resolution, though; checking..

[jayaddison]

@jayaddison please could you resolve conflicts from #11426?

Done; it looks like there may be one or two more Content-Length response headers that I missed during the conflict resolution, though; checking..

Ok, the missed content-length header has also been resolved. There was only one codesite that required updating. The other that I had been considering was this send_error call -- it turns out that for versions of Python 3.4 onwards, calling send_error will cause a content-length header to be transmitted to the client.

[AA-Turner]

Thanks @jayaddison!

A

[jayaddison]

Thank you!