-
Notifications
You must be signed in to change notification settings - Fork 24
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
Catch and expound upon the py38 proactor add_reader() not implemented exception #88
base: main
Are you sure you want to change the base?
Changes from 10 commits
b5d9a85
23171de
0c6a3f4
1537fbb
b1c063f
8e081e9
3f358b8
907a8a1
1f2b73f
bb4cb3e
4c7cf0b
98b223a
b9cc0b7
9d2639a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,30 @@ | ||
import functools | ||
import inspect | ||
import sys | ||
import traceback | ||
import warnings | ||
|
||
import decorator | ||
import greenlet | ||
import pytest | ||
import six | ||
|
||
from twisted.internet import error, defer | ||
from twisted.internet.threads import blockingCallFromThread | ||
from twisted.python import failure | ||
|
||
|
||
class AddReaderNotImplementedError(Exception): | ||
@classmethod | ||
def build(cls): | ||
return cls( | ||
"Failed to install asyncio reactor. The proactor was" | ||
" used and is lacking the needed `.add_reader()` method" | ||
" as of Python 3.8 on Windows." | ||
" https://twistedmatrix.com/trac/ticket/9766" | ||
) | ||
|
||
|
||
class WrongReactorAlreadyInstalledError(Exception): | ||
pass | ||
|
||
|
@@ -275,10 +288,27 @@ def init_qt5_reactor(): | |
def init_asyncio_reactor(): | ||
from twisted.internet import asyncioreactor | ||
|
||
_install_reactor( | ||
reactor_installer=asyncioreactor.install, | ||
reactor_type=asyncioreactor.AsyncioSelectorReactor, | ||
) | ||
try: | ||
_install_reactor( | ||
reactor_installer=asyncioreactor.install, | ||
reactor_type=asyncioreactor.AsyncioSelectorReactor, | ||
) | ||
except NotImplementedError as e: | ||
import asyncio | ||
|
||
_, _, traceback_object = sys.exc_info() | ||
stack_summary = traceback.extract_tb(traceback_object) | ||
source_function_name = stack_summary[-1].name | ||
event_loop = asyncio.get_event_loop() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm -1 on this. I realize you're trying to scope it to this specific case to avoid a lying error message, but IMO this relies way too heavily on implementation details. I think preserving the traceback is enough for context, and then just change the error to say what we think happened... Phrased like "If you're on windows and python 3.8+, you might need to..." perhaps. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps my head was still in the 'identify the issue and resolve it' mode where you want to be sure. Given this is just going to refine the error message which can be worded as a guess it is more acceptable to... guess. Maybe instead of more granular Plus I suppose back to 'this is really twisted's problem to address' justifying less effort. |
||
|
||
if ( | ||
source_function_name == "add_reader" | ||
and isinstance(event_loop, asyncio.ProactorEventLoop) | ||
): | ||
six.raise_from( | ||
value=AddReaderNotImplementedError.build(), | ||
from_value=e, | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs some There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like I said, late and sleepy... |
||
|
||
|
||
reactor_installers = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ | |
author_email="[email protected]", | ||
url="https://github.com/pytest-dev/pytest-twisted", | ||
py_modules=["pytest_twisted"], | ||
install_requires=["greenlet", "pytest>=2.3", "decorator"], | ||
install_requires=["greenlet", "pytest>=2.3", "decorator", "six"], | ||
extras_require={"dev": ["pre-commit", "black"]}, | ||
classifiers=[ | ||
"Development Status :: 5 - Production/Stable", | ||
|
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.
This error message could use some love. The proactor doesn't implement
add_reader
, period, not just on 3.8.More to the point, why the single-case exception? I think it would be better to just raise
NotImplementedError
with that message in theexcept
block.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.
The proactor might implement
.add_reader()
in the future. But sure, we know for <= 3.8.1.I like specific exceptions and not incidentally catching those I don't intend by libraries providing their own. Look at the hoops to figure out if it is the
NotImplementedError
from.add_reader()
. But I did consider inheriting fromNotImplementedError
since it is araise from
situation.