-
Notifications
You must be signed in to change notification settings - Fork 320
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
eventlet.websocket is not always used from eventlet.wsgi, so do not assume eventlet.set_idle
exists
#949
Conversation
Note that I don't know of any good way to test this inside Eventlet... it seems fiendishly complicated to mock a Gunicorn worker, so that's left as an exercise to the maintainer :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerning myself, this patch LGTM.
I'd appreciate to have a secondary opinion from Tim.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #949 +/- ##
=====================================
- Coverage 56% 56% -1%
=====================================
Files 89 89
Lines 9766 9767 +1
Branches 1818 1819 +1
=====================================
Hits 5475 5475
- Misses 3920 3921 +1
Partials 371 371
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Well, I think these changes are well isolated and should not trigger side effects. Let's go ahead to fix the latest release. Thanks @dhdaines for your patch,for your time and for the interesting discussion. Much appreciated. |
It will allow us to benefit from several recent fixes related to the new asyncio hub. We started to implement some devstack integrations with the new eventlet asyncio hub [1], so it is worth using a version that contains useful fixes. See https://review.opendev.org/c/openstack/devstack/+/914108 Also this patch propose to blacklist version 0.36.0 which is buggy version in some wsgi context. 0.36.1 fix that wsgi problem. For further details see: - eventlet/eventlet#946 - eventlet/eventlet#949 Change-Id: I2add78de0e3d2f439964afa0d0c9d854a52b5f7f
* Update requirements from branch 'master' to 4ba203d335842ba76005e0ab3a395c6b1f5853c4 - Merge "bump eventlet to the latest version" - bump eventlet to the latest version It will allow us to benefit from several recent fixes related to the new asyncio hub. We started to implement some devstack integrations with the new eventlet asyncio hub [1], so it is worth using a version that contains useful fixes. See https://review.opendev.org/c/openstack/devstack/+/914108 Also this patch propose to blacklist version 0.36.0 which is buggy version in some wsgi context. 0.36.1 fix that wsgi problem. For further details see: - eventlet/eventlet#946 - eventlet/eventlet#949 Change-Id: I2add78de0e3d2f439964afa0d0c9d854a52b5f7f
In the case where Eventlet's WebSocket implementation is used, for instance, in a Gunicorn worker, it ends up being the case that
WebSocketWSGI
is not running under an Eventlet server, and thus shouldn't assume that the environment containseventlet.set_idle
(which is an implementation detail of the Eventlet server and thus quite meaniningless).This is notably the case when using Flask-SocketIO with Gunicorn as suggested in the documentation: https://flask-socketio.readthedocs.io/en/latest/deployment.html#gunicorn-web-server
It seems quite safe to just avoid trying to call it in this case. If, for instance, the engineio wrapper (https://github.com/miguelgrinberg/python-engineio/blob/main/src/engineio/async_drivers/eventlet.py#L39) wishes to provide
eventlet.set_idle
for some reason, then it could do so.Fixes #946