Make custom queue class feature work well with gevent

[sethmlarson]

See:

• gevent and weakref.finalize potentially cause non-deterministic deadlock gevent/gevent#1957 (comment)
• gevent and weakref.finalize potentially cause non-deterministic deadlock gevent/gevent#1957 (comment)

Basically us holding on to a reference to an unpatched queue.LifoQueue is getting in the way of gevent's monkey-patching. If we instantiated queue.LifoQueue without using our QueueCls reference in the default case it likely wouldn't break gevent users.

I'm unsure what the exact implementation would look like, open to suggestions.

[sigmavirus24]

So I don't know if anyone swaps out the class today anywhere. If they do, we could have a class property setter and getter such that the getter returns this without setting the value if the hidden attr is unset, otherwise we return the value. That would allow the patches version to be referenced dynamically in the getter method. Having the setter then keeps backward compatibility for those that need it

[ZLotusRain]

In my case,after upgrade urllib3 to 1.26.16 i met the same deadlock or the stuck situation,and after digging into the gevent and urllib3 and other libraries which are using gevent and weakref.finazlize like celery, i think the problem is related to the implemention of Queue(maybe and Lock).

As far as i know, gevent patched Queue with SimpleQueue,but it seems has no effect on urllib3.ConnectionPool which skillfully imports the right Queue according to the Python version and for me is Python3.

So after patched and before using urllib3(accurately before using urllib3.ConnectionPool), i replaced the QueueCls(by default is urllib3.util.queue.LifoQueue) with gevent.queue.LifoQueue which can take advantage of the fact that certain operations cannot be interrupted by other greenlets, and after hundreds turns of running my random test suites it still seems fine.

Whether we can add a function like urllib3.patch_xxx like gevent does, and let PoolManager to manage this.

[zawan-ila]

Could we just set ConnectionPool.queueCls to None and inside of HttpConnectionPool.__init__ do self.queueCls or queue.LifoQueue?

[sethmlarson]

@zawan-ila I was imagining something like that, but maybe we can save the original LifoQueue instance and do an "is" check before attempting to reimport from the queue module? Does that make sense?

[sigmavirus24]

I'm not even certain laziness wins here since on 1.26 we're importing Queue from six and implementing our own LifoQueue

[kchoudhu]

Is this something gevent can fix by patching queue.LifoQueue with the equivalent gevent version during the patch process?

[sigmavirus24]

@kchoudhu I'm certain they already do that.

To reiterate, on 2.x:

• We import queue (the module) and then we assign the class to a class property ref
• If gevent.monkey_patch_all() happens after someone imports urllib3, gevent will replace queue.LifoQueue but we've stored a reference to the original, so we don't get that.

On 1.26.x:

• We have our own LifoQueue.
• We rely on six.moves.queue which I think gevent has dropped support for python 2.7 and six
• And when we declare our LifoQueue class like this before gevent.monkey_patch_all() happens after urllib3 is imported, the LifoQueue parent will be the Queue that isn't monkey-patched.

So the problem can be solved by users importing gevent and monkey-patching first, but we know that isn't as simple as it sounds. So if there's a different thing we can do, that would be the best option.

It's likely we cannot fix this for 1.26.x as easily, but for 2.x I think what we can do is something like:

class ConnectionPool:
    # ...
    QueueCls: t.Type[queue.LifoQueue] | None = None
    # ...
    def _make_queue(self, *args, **kwargs) -> queue.LifoQueue:
        if self.QueueCls is None:
             return queue.LifoQueue(*args, **kwargs)
        return self.QueueCls(*args, **kwargs)


class HTTPConnectionPool(ConnectionPool, RequestMethods):
    # ...
    def __init__(
        # ...
    ) -> None:
        # ...
        self.pool: queue.LifoQueue[typing.Any] | None = self._make_queue(maxsize)

That would allow monkey-patching to swoop in after us and replace things for us. But again, that's only 2.x

[kchoudhu]

Looking at the gevent codebase, it doesn't appear that all of the queue module is patched out, just SimpleQueue:

https://github.com/gevent/gevent/blob/master/src/gevent/monkey.py#L543

Even with an early enough patch_all(), urllib3 would still get the base python LifoQueue.

Agree that the change for 2.x will allow for a more comfortable queue choice experience -- thank you!

[ZLotusRain]

Looking at the gevent codebase, it doesn't appear that all of the queue module is patched out, just SimpleQueue:

https://github.com/gevent/gevent/blob/master/src/gevent/monkey.py#L543

Even with an early enough patch_all(), urllib3 would still get the base python LifoQueue.

Agree that the change for 2.x will allow for a more comfortable queue choice experience -- thank you!

it's the exact thing i mentioned here:
#3289 (comment)
and aslo in this issue:
gevent/gevent#1957 (comment)

and i prefer to add the patch funtion in gevent, because we can't rely on any other packages which are using urllib3 to provide some interfaces to change the queue class