-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make custom queue class feature work well with gevent #3289
Comments
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 |
@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 I'm certain they already do that. To reiterate, on 2.x:
On 1.26.x:
So the problem can be solved by users importing 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 https://github.com/gevent/gevent/blob/master/src/gevent/monkey.py#L543 Even with an early enough 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: 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:
Its possible that setting the queue implementation to 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. |
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 instantiatedqueue.LifoQueue
without using ourQueueCls
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.
The text was updated successfully, but these errors were encountered: