Hi @jku, I implemented a FetcherInterface that utilizes urllib3, most of the implementation is quite similar to RequestsFetcher. For the tests, I swapped out the RequestsFetcher for the Urllib3Fetcher in test_fetcher_ng.py. (I haven't removed the requests_fetcher.py yet)

On first pass this looks really good, thanks -- I was worried there might be some differences that makes this complicated but your work seems very promising. Still, we'll have to be careful not to change any details we don't intend to as this is very core client code that easily affects applications (I mention this in case the review takes a while: it's only because I know this is a potentially tricky change).

I think we can probably even drop the multiple poolmanagers: this manual connection pool management was added at a time when this wasn't part of the http library but now I believe the poolmanager handles exactly the issues the _get_poolmanager() code is working around. I'm going to review this better in the next days so I'll look into this.

I think we can probably even drop the multiple poolmanagers: this manual connection pool management was added at a time when this wasn't part of the http library but now I believe the poolmanager handles exactly the issues the _get_poolmanager() code is working around. I'm going to review this better in the next days so I'll look into this.

sounds good, I was looking at this page and I think it is supported, so I can remove _get_poolmanager() / _poolManagers and just use one of them. I was wondering if I should pass in a different value for the num_pools parameter, or if the default amount of 10 works.

For the record I was looking at the timeout results with these simple tests:

import urllib3
# Test connection timeout: MaxRetryError caused by NewConnectionError
urllib3.request("GET", "https://github.com", timeout=urllib3.Timeout(connect=0.0001))
# Test read timeout: MaxRetryError caused by ReadTimeoutError
urllib3.request("GET", "https://github.com", timeout=urllib3.Timeout(read=0.0001))

(note that this simple test reuses the default connection pool so a successful connection will prevent retesting the connection timeout without reloading the module)

documenting items for changelog:

• Default FetcherInterface implementation is now Urllib3Fetcher instead of RequestFetcher
  • In practice this uses urllib3 instead of requests for HTTP
  • this change also switches the TLS certificate source from certifi to system certificate store
  • RequestsFetcher is deprecated but remains available for at least this release

This feels like a major version bump regardless of the actual API changes

we probably want to provide RequestsFetcher for at least one release

we can't remove certifi/requests depencies yet (since RequestsFetcher is still in the API)

Hmm, maybe we could not depend on requests and just document that users who want to use RequestFetcher can do that... If that works in practice that might be a reasonable compromise

Anyway, I'm ok with this PR not touching the requirements: There are other uses of requests still left in the code base (examples etc) that we would want to modify before that anyway

Anyway, I'm ok with this PR not touching the requirements: There are other uses of requests still left in the code base (examples > etc) that we would want to modify before that anyway

gotcha, I omitted requester_fetcher.py in pyproject.toml, as it was causing code coverage to be < 97%. Also, there was a deprecation warning when running the test suite, but the tests in the code base were intentionally using no scheme/host to trigger error handling logic.

Cheers, I think this looks good for next release

• I've filed the followup issues (More porting from requests to urllib3 #2773, silence a deprecation warning in tests #2776, Update deps and docs after urllib3 migration #2777)
• Would like another reviewer to have a look as well
• I'll try to understand the response release mechanisms a little better -- I'm not sure I still fully understand the Response.close() and Response.release_conn() details