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

Catch and expound upon the py38 proactor add_reader() not implemented exception #88

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 34 additions & 4 deletions pytest_twisted.py
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"
Copy link
Collaborator

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 the except block.

Copy link
Member Author

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 from NotImplementedError since it is a raise from situation.

)


class WrongReactorAlreadyInstalledError(Exception):
pass

Expand Down Expand Up @@ -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()
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 NotImplementedErrord raisers should include the function object raising it for identification.

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,
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs some else: raise otherwise you suppress the exception

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I said, late and sleepy... :| Will do.



reactor_installers = {
Expand Down
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
63 changes: 63 additions & 0 deletions testing/test_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,34 @@ def format_run_result_output_for_assert(run_result):
)


def proactor_add_reader_is_implemented():
try:
import asyncio

add_reader = asyncio.proactor_event_loop.add_reader

try:
add_reader(None, None, None)
except NotImplementedError:
return False
except: # noqa: E722
return True
except: # noqa: E722
return False


def proactor_is_default():
try:
import asyncio

return (
asyncio.DefaultEventLoopPolicy
is asyncio.WindowsProactorEventLoopPolicy
)
except: # noqa: E722
return False


@pytest.fixture(name="default_conftest", autouse=True)
def _default_conftest(testdir):
testdir.makeconftest(textwrap.dedent("""
Expand Down Expand Up @@ -696,3 +724,38 @@ def test_succeed():
testdir.makepyfile(test_file)
rr = testdir.run(sys.executable, "-m", "pytest", "-v", *cmd_opts)
assert "WrongReactorAlreadyInstalledError" in rr.stderr.str()


@pytest.mark.skipif(
condition=sys.platform != 'win32',
reason="Relevant only on Windows",
)
@pytest.mark.skipif(
condition=proactor_add_reader_is_implemented(),
reason=(
"Relevant only if asyncio.ProactorEventLoop.add_reader()"
" is not implemented"
),
)
@pytest.mark.skipif(
condition=not proactor_is_default(),
reason="Proactor is not the default event loop policy."
)
def test_add_reader_exception_expounded_upon(testdir, cmd_opts, request):
skip_if_reactor_not(request, "asyncio")

# block the default conftest.py
testdir.makeconftest("")
test_file = """
def test_succeed():
pass
"""
testdir.makepyfile(test_file)
rr = testdir.run(sys.executable, "-m", "pytest", "-v", *cmd_opts)
assert all(
s in rr.stderr.str()
for s in [
"AddReaderNotImplementedError",
"https://twistedmatrix.com/trac/ticket/9766",
]
)