Make custom queue class feature work well with gevent

See:

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.

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

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.

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

@zawan-ila

@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?

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

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

@kchoudhu

@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

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!

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

I don't believe that LifoQueue patching is the problem. Many different variations of this have been reported, and you can see in the stacktraces that the lock implementation is gevent monkey-patched, for example, from #3061:

/usr/lib/python3.9/threading.py:274: in _is_owned
    if self._lock.acquire(False):
../../.virtualenvs/rotkipy39/lib/python3.9/site-packages/gevent/thread.py:141: in acquire
    sleep()

Its possible that setting the queue implementation to gevent.queue.LifoQueue is causing it to no longer repro because that is a totally different implementation than stdlib Queue (subclassed by urllib3 LifoQueue). This issue is quite intricate to repro, and this repro case happens to do it with the stdlib Queue, but might not with gevent.queue.LifoQueue.

What would urllib3 devs think about an implementation of _close_pool_connections that didn't call any gevent APIs? It couldn't call queue.get, so it would need to be redesigned a bit.

Hey! Is this issue still available for an external contributor, and is the $100 OpenCollective bounty still active? I’m interested in working on it with coding-agent assistance and will be responsible for reviewing the submission. If it remains available, should the fix target the current main branch and include a regression test covering gevent monkey-patching?

Just submitted the expense. Really happy I got to contribute to urllib3! And having Seth approve it honestly made my day haha. Thanks again! Looking forward to helping out again.

Navigation Menu

Repository navigation

Make custom queue class feature work well with gevent #3289

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Make custom queue class feature work well with gevent #3289

Description

Activity

sigmavirus24 commented on Jan 20, 2024

ZLotusRain commented on Jan 20, 2024

zawan-ila commented on Jan 20, 2024

sethmlarson commented on Jan 20, 2024

sigmavirus24 commented on Jan 20, 2024

kchoudhu commented on Mar 14, 2024

sigmavirus24 commented on Mar 14, 2024

kchoudhu commented on Mar 15, 2024

ZLotusRain commented on Mar 15, 2024

jaklaassen-affirm commented on Sep 9, 2024

atdpdx commented on Jul 23, 2026

ImAurum11 commented on Aug 11, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions