eventlet worker: ALREADY_HANDLED -> WSGI_LOCAL #2581
Conversation
Fix issue with import error while using latest eventlet (>=0.30.3). Changes made in eventlet: benoitc#2581
* develope: (69 commits) eventlet 0.31.0 still has bug. return to 0.30.2 benoitc/gunicorn#2581 Remove pip package gunicorn version project_members_detail response add pm_user_login * Update project.py apiError.py 1. check patterns exist in project description 2. add invalid_project_description add return data: ' ' when the socket return bug fixed. delete multi role binding (user, project) when delete user. Don't keep git-count-commits and job by edit cronjob yaml Unify project.repository_ids to array type Don't keep sync-redmine and job by edit cronjob yaml * Update sync_redmine.py 1. set default admin_account 2. insert_project add pm_user_login 3. insert_all_issues add assigned_to_login 4. get_redmine_issue_rank add user_login *Update model.py 1. RedmineIssue add assigned_to_login 2. RedmineProject add pm_user_login Bump eventlet from 0.30.2 to 0.31.0 fix bug and format for PE8 fix page_search number fix ad search citeria Fix bug Can change role of users from ad Fix correct location timeout to avoid timeout while creating database Change SocketIO timeout to 60s Change SocketIO timeout to 60s ...
|
Can we have at least a basic agreement of not doing such breaking changes as part of a patch version upgrade at least |
Agreed, however a major version change would have had the same result:
|
Then what is the use of semver? |
In theory you would pin the version using something like You might consider opening a pull request suggesting your way of managing the versions to see if the project will accept it or consider moving to a dependency manager that uses a lock file (poetry?) but what's more concerning to me is the delay on reviewing and merging this PR. |
💯, it would be great to see some follow-up from the gunicorn folks about this. |
I am pinning the eventlet version in my requirement file and dependency management is not the point I am talking about, my issue is that since it is a security vulnerability(which again by definition should have warranted a MAJOR or MINOR version bump) so I expected gunicorn would also patch it up from their end given that eventlet is an optional dependency for gunicorn.
Even I am puzzled that there had been no acknowledgement from their end on this PR. |
gunicorn/workers/geventlet.py
Outdated
| @@ -125,6 +130,10 @@ def patch(self): | |||
| patch_sendfile() | |||
|
|
|||
| def is_already_handled(self, respiter): | |||
| # eventlet >= 0.30.3 | |||
| if EVENTLET_WSGI_LOCAL and EVENTLET_WSGI_LOCAL.already_handled: | |||
There was a problem hiding this comment.
why do we need an « and » there ? can’t we simply check if one or the other is present and set to true?
There was a problem hiding this comment.
Sorry, I don't understand.
We need first check because there may be older version of eventlet and module level getattr would set it to None. Arguably should explicitly compare to None like so.
if EVENTLET_WSGI_LOCAL is not None: # eventlet >= 0.30.3
And then if we know it's eventlet with WSGI_LOCAL, we can check its already_handled and avoid resolving None.already_handled.
Surely you got all this and I'm just confused about what other option is possible here. Sorry. If you show a more concrete example, I will change code as you wish.
There was a problem hiding this comment.
I think another way of expressing this would be to write if getattr(EVENTLET_WSGI_LOCAL, 'already_handled', None):.
I am not saying this is better, I am just stating @temoto's case another way, to help illustrate the intent. (Or at least, that's how I undersatnd it.)
Edit: I re-considered and realized that my initial take was wrong, so updated my snippet.
There was a problem hiding this comment.
@benoitc did you want getattr like that?
There was a problem hiding this comment.
I actually believe it needs to be: if EVENTLET_WSGI_LOCAL and getattr(EVENTLET_WSGI_LOCAL, 'already_handled', False):
without that I get AttributeError: 'local' object has no attribute 'already_handled'
There was a problem hiding this comment.
Changed to single getattr, it would work in earlier versions of eventlet.
i commented about the PR (thanks for it). Please have a look About semver or not I do think we shouldn’t as a project rely too much on it. Sometimes an author is just wanting to ensure an hotfix is going out. We should rather improve our testing to ensure the teste pass when a new dependency is out. It would be cool if we can do that automatically. Possibly without relying on a proprietary service. Any idea/hint is welcome :) |
Yes, I will setup Eventlet CI with gunicorn. |
because a) we're not doing stuff with websockets, so we're not very vulnerable b) the fixed version of eventlet does not work with gunicorn yet: benoitc/gunicorn#2581 also fixes typo in requirements regarding this point Co-authored-by: Saman Moshafi <[email protected]> Co-authored-by: Vikram Kohli <[email protected]>
|
Why is there no movement on this? |
|
I've referenced this issue from #2828, which appears to be the most up-to-date open issue on the topic of creating a new release. |
Issue: benoitc/gunicorn#2581 Also add git to dockerimage.
this requires a patched eventlet, see |
I'm tired of waiting for this fix to be released and the I'm tired of the gunicorn author saying "oh yeah any day now" for nearly two years. benoitc/gunicorn#2581
|
New release, please. We can't afford to use in production a project that takes more than two years to fix an obvious bug and release a new version, especially when the workaround involves using a previous vulnerable version of eventlet. And using a Github link for a dependency is a dirty workaround that nobody should be forced to do in production. If maintainers can't maintain this anymore, please make room to somebody else. Nobody deserves to beg for a new release for two years. Is it safe to assume that this project is no longer maintained, so we can move on to alternatives? |
Reason: gunicorn maintainers no longer give a fuck about their project and they aren't letting anybody take over either - see benoitc/gunicorn#2581 This is not how a FOSS project should be run. A project with 9k stars and countless usages shouldn't end up in a situation where users beg for two years for a new release that fixes a bad regression and a bad security vulnerability. The way gunicorn is maintained and run is an insult to the whole FOSS community.
|
@blacklight did you test master. Does this change works for you? Next time please come with a more constructive comment. |
@benoitc, if you have the time to reply this comment, at least action #2828 and release a new tagged release WITH this PR in it. You'll notice magically everyone will stop pestering you about this issue over and over again... |
|
@bt this issue is closed. |
For some reason everything works out of the box on Arch Linux using the provided pacman packages - I'm not sure if the maintainers of the eventlet or gunicorn packages already provided a patch. It also works on a fresh virtual environment with the latest pip versions of eventlet and everything else. I still haven't found a way of getting things to work on Debian Bullseye, but I guess it may be due to some of the ancient |
|
Any resolution to this? Still appears to be an issue in the latest eventlet version. |
|
@benoitc, I tested Maybe if it is easy to copy and paste a few more will test. ;) |
|
Confirmed working for me when directly referencing the github link (thanks CurtisLeeBolin) in my requirements file. Had to add 'apt-get -y git' in my Dockerfile for it to build, but at least the issue found in the PyPI release is no longer occurring. |
|
@imalobster you didn't need git. You can add by it's zip like this: https://github.com/benoitc/gunicorn/archive/refs/heads/master.zip |
Gunicorn 21.x includes this benoitc/gunicorn#2581 which allow us to use recent eventlet versions.
Gunicorn 21.x includes this benoitc/gunicorn#2581 which allow us to use recent eventlet versions.
Gunicorn 21.x includes this benoitc/gunicorn#2581 which allow us to use recent eventlet versions.
|
FYI gunicorn 21.0.0 was released on 18/jul/2023 and contains this fix. |
|
@mikeyatmixpanel thanks! I've done some local tests and the new version seems to work. My project that needed a gunicorn WSGI wrapper a couple of years ago has already been migrated to Tornado in the meantime (I couldn't afford to use non-pip dependencies for it), but I'll consider gunicorn again for any next projects. Are there any plans to backport the fix also to previous versions of gunicorn? It'll probably take a while before 21.0 gets adopted by major package managers, and any distros stuck on eventlet >= 0.30.3 and gunicorn < 21.0.0 would definitely benefit from this fix. |
|
That's more of a question for @benoitc. Any chance of patch releases 20.1.1 and 20.0.5? |
|
The new version works great for the MSS project. @mikeyatmixpanel usually package maintainers can do such a backport on their own. I am not sure if they do that when they also have to change the packaged eventlet version. Two month ago v22.0.0 was added to conda-forge. 21.2.0 is the recent version on https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/python-modules/gunicorn/default.nix When you enable nix packages you may get that version installed or fallback to conda-forge. |
Eventlet 0.30.3+ removed
wsgi.ALREADY_HANDLEDbreaking public API in just a patch version increase. Sorry.Issue with ALREADY_HANDLED: eventlet/eventlet#543
Solution with WSGI_LOCAL: eventlet/eventlet#544
It's recommended to use
eventlet>=0.31.0if one uses websockets, because older versions are vulnerable to DoS attack. GHSA-9p9m-jm8w-94p2CI failed in pylint checks on lines I didn't touch.