Skip to content
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

Monkey-patch ssl in __init__ to fix grequests warning/error #197

Merged
merged 1 commit into from
Aug 17, 2024

Conversation

tianyizheng02
Copy link
Contributor

The grequests package monkey-patches ssl upon import, but this must be done before all other imports. This PR imports gevent at the very top of __init__.py and explicitly monkey-patches everything.

Currently, without this patch, importing the news module will raise a MonkeyPatchWarning:

>>> from pittapi import news
.../lib/python3.12/site-packages/grequests.py:22: MonkeyPatchWarning: Monkey-patching ssl after ssl has already been imported may lead to errors, including RecursionError on Python 3.6. It may also silently lead to incorrect behaviour on Python 3.7. Please monkey-patch earlier. See https://github.com/gevent/gevent/issues/1016. Modules that had direct imports (NOT patched): [...]. 
  curious_george.patch_all(thread=False, select=False)

while importing textbook will raise this warning as well as a RecursionError:

>>> from pittapi import textbook
.../lib/python3.12/site-packages/grequests.py:22: MonkeyPatchWarning: Monkey-patching ssl after ssl has already been imported may lead to errors, including RecursionError on Python 3.6. It may also silently lead to incorrect behaviour on Python 3.7. Please monkey-patch earlier. See https://github.com/gevent/gevent/issues/1016. Modules that had direct imports (NOT patched): [...]. 
  curious_george.patch_all(thread=False, select=False)
Traceback (most recent call last):
...
RecursionError: maximum recursion depth exceeded

With this patch, both modules can be imported with no warnings or errors.

References:

The grequests package monkey-patches ssl upon import, but this must be
done before all other imports or else we get a MonkeyPatchWarning or a
RecursionError.

References:
spyoungtech/grequests#150
gevent/gevent#1016
@tianyizheng02
Copy link
Contributor Author

flake8 raises errors about imports not being at the top of the file:

./pittapi/__init__.py:27:1: E402 module level import not at top of file
./pittapi/__init__.py:28:1: E402 module level import not at top of file
2     E402 module level import not at top of file
2

This is true, but it's also entirely intentional, since we need to monkey-patch before all other imports, including other imports in __init__.py.

@tianyizheng02
Copy link
Contributor Author

tianyizheng02 commented Aug 16, 2024

Also, for some reason these monkey-patching warnings/errors only appear when you import modules normally, and they don't occur when modules are imported during tests. Not sure why this is, but we could've caught this issue much sooner had these warnings/errors appeared during tests.

@tianyizheng02 tianyizheng02 merged commit f9f3def into dev Aug 17, 2024
4 checks passed
@tianyizheng02 tianyizheng02 deleted the monkey-patch-grequests branch August 17, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants