tests: linkcheck: add client connection-pool contention coverage

[jayaddison]

Feature or Bugfix

• Test coverage

Purpose

• Provides test coverage for implementations attempting to resolve linkcheck builder: begin using requests.Session functionality during linkchecking #11324.

Detail

• Configure an artificially-limited connection pool by patching requests.Session.get_adapter
• Create a multi-item linkcheck workload
• Consume one item at a time from the workload from separate threads
• Ensure that all items were consumed and that no timeouts occurred

Relates

• Prepares for linkcheck builder: begin using requests.Session functionality during linkchecking #11324.
• Planned from Flaky unit test: test_auth_header_uses_first_match #11348 (comment).

[jayaddison]

Why does the test_connection_contention test case on Windows only manage to perform 4 of the 10 linkchecks before the 5-second timeout occurs?

[jayaddison]

I'd like to remove this because I think it has side-effects that outlast the unit test.

It was introduced to avoid indefinitely-blocked socket I/O when the test linkcheck client and server fail to communicate reliably.

[francoisfreitag]

I believe this could be a global setting for the test suite, I don’t see why an indefinitely-blocked socket I/O would be reasonable in that context.

[jayaddison]

Yep, that would make sense. It would also be nice if the tests could fail if that timeout limit is ever reached, because that'd almost certainly be a sign of a problem with the application code, given that we now entirely request local, non-Internet resources during tests (I believe that's true?).

Another relevant note is that, in practice, I think we already approximate this behaviour -- perhaps unintentionally -- from the time at which a linkchecker object is first constructed. That's because there is a call to the same setdefaulttimeout method here:

sphinx/sphinx/builders/linkcheck.py

Lines 119 to 120 in d48cc78

	# set a timeout for non-responding servers
	socket.setdefaulttimeout(5.0)

It does probably make sense to have a timeout in production environments, because in most cases it would be bad for a single long-delayed link to block the entire linkcheck process, but at the same time it would be good to confirm why the global timeout is in place given that there is an existing -- and configurable -- linkcheck timeout setting:

sphinx/sphinx/builders/linkcheck.py

Lines 270 to 271 in d48cc78

	if self.config.linkcheck_timeout:
	kwargs['timeout'] = self.config.linkcheck_timeout

(I also should explain that it seems OK to me that both timeouts exist: I sometimes have a tendency to remove redundancy in software to such an extent that the resulting code becomes fragile in other ways, and I'm keen that we avoid that risk here. but I think it would be good to understand the reasoning why both exist. I think it's basically to try to avoid the linkchecker ever stalling indefinitely, even in the absence of a configured linkcheck_timeout value)

[francoisfreitag]

I agree with you. My reading of the code (and https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-linkcheck_timeout) is:

1. linkcheck_timeout=None: make sure linkcheck bails out after a reasonable amout of time (5.0 seconds), since requests won’t handle the timeout
2. linkcheck_timeout=3.14: specify the timeout to requests

So, by default, linkcheck will not get stuck. When the default does not fit a project (developers want slower or longer timeouts), they can use linkcheck_timeout. I don’t see a way to specify no timeout, but I don’t think that’s an issue. A noteworthy surprise is that setting linkcheck_timeout=5.0 is different from using the default, as the default only matters for the socket connection, whereas requests timeout also considers the time to generate the response on the server.

I think the side-effect is not intentional but is pretty nice to have and should not be changed. There aren’t other socket.setdefaulttimeout() in the app, so linkcheck sets that default for other extensions (thinking of extlinks). My suggestion is to move the socket.setdefaulttimeout() to sphinx.application.Sphinx.__init__(). That makes the call explicit and should not break BC (since the socket.setdefaulttimeout() happened when the extensions were being setup, as setup_extension imports the module).

I believe the test suite could use the same setting as the application, or even a shorter one, considering that waiting 5 seconds is pretty long in that context. Anything above a second would probably indicate an issue IMO.

Coming back to linkcheck, we could remove the call to socket.setdefaulttimeout() (that has a side effect on the Sphinx application), and set a default of 5.0 for linkcheck_timeout, but it would break BC:

• https://docs.python-requests.org/en/latest/user/advanced/#timeouts shows socket.setdefaulttimeout() will timeout only on what requests calls “connect”, not while reading data from the server. So a user who connected quickly but waited a long time to receive the response would see failures where it previously worked. I’m guessing we could maybe (and I don’t wanna) abuse the current implementation to pass a tuple (5.0, None) and preserve the current behavior.
• users may call socket.setdefaulttimeout() after Sphinx.__init__(), to use their own values. That would be a bit surprising considering linkcheck_timeout exists, but who knows...

[francoisfreitag]

That makes the call explicit and should not break BC (since the socket.setdefaulttimeout() happened when the extensions were being setup, as setup_extension imports the module).

That’s actually not true. socket.setdefaulttimeout(5.0) is only called during CheckExternalLinksBuilder.__init__(), which happens when the builder is created, not when the module is imported and the builder registered.

[jayaddison]

I agree with you. My reading of the code (and https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-linkcheck_timeout) is:

1. `linkcheck_timeout=None`: make sure linkcheck bails out after a reasonable amout of time (5.0 seconds), since `requests` won’t handle the timeout

I'm not so sure about this - my reading of the documentation:

linkcheck_timeout

A timeout value, in seconds, for the linkcheck builder. The default is to use Python’s global socket timeout.

New in version 1.1.

... is that the unmodified global timeout for sockets is to be expected unless the operator changes the configuration value.

What we have currently is a 5-second timeout set by the application when no configuration is applied. I do think that having a timeout in production makes sense, but either the documentation or the code may need updating to bring them into consistency.

It'll take me a while longer to process the rest of your reply and to think about any further changes.

[jayaddison]

I'll create a separate issue soon to track resolution for this (relocation and/or removal of the socket.setdefaulttimeout calls).

[jayaddison]

@francoisfreitag could you check the approach I'm using in this pull request against the planned coverage?

Add a test using a pool with a single connection in the pool, and a large-but-not-too-large timeout (e.g. 1s) to exhibit the connection pool exhaustion from not response.close()-ing?

At the moment, it passes using HTTP/1.0 test webserving, as expected.

It also fails with HTTP/1.1 testserving, as expected.

However: I find that it fails (unexpectedly) when both HTTP/1.1 webservers and session-based linkchecking (that should response.close...) are enabled.

[jayaddison]

Thanks @AA-Turner!