Add urllib3.contrib.socks; improve urllib3.connectionpool

[kkirsche]

This is required to address some re-exports in #8439

The source code is available here:
https://github.com/urllib3/urllib3/blob/main/src/urllib3/contrib/socks.py

[AlexWaygood]

As well as @srittau's points, note that this module only imports successfully if the optional dependency PySocks is installed (according to https://github.com/urllib3/urllib3/blob/16d2976041c5b8b0fe5f790e8edc9c50f318ee7b/src/urllib3/contrib/socks.py#L50-L52). You'll need to add the optional dependency to a requirements-stubtest.txt file in order for stubtest to be able to import it in CI (see https://github.com/python/typeshed/blob/master/stubs/tqdm/%40tests/requirements-stubtest.txt for an example).

[AlexWaygood]

I still think this would be better as the stdlib Queue[Any]. It's true that at runtime, ConnectionPool.QueueCls is urllib3.util.queue.LifoQueue. But ConnectionPool is a base class for other ConnectionPool classes, and I don't think (from my impression, from reading the code) that it's mandatory for all subclasses to have their QueueCls attribute be a subclass of urllib3.util.queue.LifoQueue. It seems to me that anything with an interface similar to the stdlib queue.Queue class will do. There's evidence for this in the fact that they scrapped the urllib3.util.queue submodule in the main branch and switched to using the stdlib queue module.

[AlexWaygood]

Anyway, this is minor, other than this it looks good to me now :)

[kkirsche]

👍 I think I got mixed up between the type[_QueueT] change and the Queue[Any] change. I agree with you on that.

If you are comfortable with it, I will update this to: ClassVar[type[Queue[Any]]]

and change the imports to be fully qualified to differentiate between the two different queues.

Sound good?

[AlexWaygood]

That sounds good to me! 👍

[kkirsche]

That was changed in 3e231f4 (I expect a pre-commit will trigger). The only thing I may have missed was if there was a better way to import the LifoQueue to avoid the overlapping module names. As it stands, I've updated it to import queue for the standard library instance and usage as queue.Queue[Any] and have left the urllib3 LifoQueue as from .util.queue import LifoQueue.

My concern was that from .util import queue as _queue would be even more confusing with both queue.Queue and _queue.LifoQueue given that both have a LifoQueue

Hopefully thats correct

[AlexWaygood]

How about from .util import queue as urllib3queue? And then refer to it as urllib3queue.LifoQueue

[kkirsche]

I had thought that was going to trigger a stubtest warning about it not existing at runtime, but I could just be getting confused about what is checked and what isn't.

That's a good approach, thank you for the suggestion, worst case we can do it as from .util import queue as _urllib3queue to avoid the check

[AlexWaygood]

All imports in stub files are considered "private to the stub" (not re-exported), unless they fall into one of the two categories:

• import foo as foo -- the as has to be truly redundant; if you do import foo as bar, it's still considered private, but import foo as foo is considered a public re-export.
• from foo import *

[AlexWaygood]

-- A third exception is if a name is included in __all__, it's always considered re-exported, even if it's imported via a means that would usually make it a "private" import.

[github-actions]

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Changed since my review.

[srittau]

Not an in-depth review, but everything looked fine at first glance. I'll defer to @AlexWaygood's judgement.

[AlexWaygood]

Let's land it. Thanks for working on this @kkirsche, this was a tricky one in lots of ways!

[kkirsche]

My pleasure, trying to work towards larger and loftier goals, so hopefully experiences like this will help