Fix AsyncioSelectorReactor on Windows Python 3.8+ #1338

rodrigc · 2020-07-05T01:19:56Z

https://twistedmatrix.com/trac/ticket/9766
https://twistedmatrix.com/trac/ticket/9809

twisted.internet.asyncioreactor.AsyncioSelectorReactor has been changed to use asyncio.SelectorEventLoop

if asyncio.get_event_loop() does not return a SelectorEventLoop.
This fixes AsyncioSelectorReactor on Python 3.8+, where
asyncio.get_event_loop() was changed to return a ProactorEventLoop
in bpo-34687. See: python/cpython@37aae9d.

adiroiban

I have never used asyncio but just by reading the documentation this doesn't look right.

I think that Twisted should use the global asyncio event loop instead of creating a separate event loop.
I guess that this is the purpose of test_defaultEventLoopFromGlobalPolicy which now fails.
This PR is green as the test was skipped.
But I think that the test should not be skipped and instead the code should be updated to have it pass.

Now I don't know if Twisted should auto set the selector policy.

Maybe is best to follow the Tornodo example and have each user set its own policy.

Just my feedback... feel free to ignore it.

Thanks!

azure-pipelines/windows_test_jobs.yml

src/twisted/internet/test/test_asyncioreactor.py

src/twisted/internet/asyncioreactor.py

src/twisted/internet/test/test_asyncioreactor.py

adiroiban · 2020-07-07T11:07:36Z

src/twisted/internet/test/test_asyncioreactor.py

    def test_seconds(self):
        """L{seconds} should return a plausible epoch time."""
+        if hasWindowsSelectorEventLoopPolicy:


I think that all the tests using _test_reactor should e moved into a separate test case.
And the AsyncioSelectorReactorTests test case can have a setup method in which it will set WindowsSelectorEventLoopPolicy and reset on teardown.

I agree, but I don't think it's necessary to hold up the PR for this.

It should at least have been a context manager or try/finally since presently it will just not reset the policy for any failing test... plus I'm already going to go copy/paste this stuff over into #1263 just to catch up.

src/twisted/internet/asyncioreactor.py

adiroiban

It looks good. Thanks!
I left some minor comments.
The final review should be done by someone using asyncio

rodrigc · 2020-08-03T19:23:37Z

@adiroiban can you have another look? I don't think that many people are using Twisted with Python's asyncio, because:

Python's asyncio works on Python-3 only, and many Twisted users were on Python 2 for a long time
Python's asyncio has been changing in every Python release

The main intent of this PR is to just keep up with all the changes in Python's asyncio, so that for people who want to use it in Twisted, they will have a better time, especially on Windows.

adiroiban · 2020-08-03T19:31:01Z

It looks good to me. But I am not using async io. I can't check things in a real production environment.

If there is no other twisted core developer interested in asyncio I think that is best to move this code outside of twisted/twisted.

rodrigc · 2020-08-03T20:07:56Z

This PR affects a reactor implementation. Reactor implementations typically have been implemented in the core of Twisted, not outside.

altendky · 2020-09-05T00:24:45Z

@rodrigc, what did you change? Given it was a force repush of all the commits, is there some way for me to see the modifications?

altendky

Since things are still changing, here's what I've got for now. I can follow up after these points are gone through and other in-work changes are done.

altendky · 2020-09-04T16:54:12Z

src/twisted/conch/test/test_knownhosts.py

@@ -1186,6 +1188,8 @@ class FakeObject(object):
    """


+
+@skipIf(not FilePath("/dev/tty").exists(), "Platform lacks /dev/tty")


Did this PR trigger this to be needed? It's not clear to me why it would.

For some reason, this test started failing, so I added it to make further progress on this PR.

There have been hundreds of lines of changes in trunk recently, so I have been updating this PR to stay current.

We should probably know why it is needed if it is going to be included?

This change looks odd to me. Those tests mock the open function, so how is it failing? If you have a reference to the failure you saw please file a ticket; there may be something wrong with the suite.

Do the tests still fail without this change?

Follow up ticket: https://twistedmatrix.com/trac/ticket/9966

src/twisted/internet/asyncioreactor.py

src/twisted/internet/test/test_asyncioreactor.py

altendky · 2020-09-05T00:36:21Z

azure-pipelines/windows_test_jobs.yml

@@ -32,4 +32,3 @@ jobs:
        platform: windows
        pythonVersion: ${{ pyver.value }}
        windowsReactor: ${{ parameters.reactor }}
-        allowFailure: ${{ eq(pyver.value, '3.8') }}


I don't see any benefit in crossing this with PR #1259 and ticket 9809.

This is a minor one line change which simply removes ignoring failurs on Python 3.8
during Windows CI. It's OK to leave this in here and address ticket 9809 at the same time.

If there weren't an existing PR addressing the existing ticket, sure. I don't get why it needs to be confused by mixing it in here.

rodrigc · 2020-09-05T03:23:34Z

@altendky I incorporated your review feedback

…rror if not passed an asyncio.SelectorEventLoop In Python 3.8+, asyncio.get_event_loop() was changed to return a asyncio.ProactorEventLoop in bpo-34687, which is incompatible with AsyncioSelectorReactor.

twm

Please fill in the missing docstring. Otherwise this looks okay to me, with the caveat that I am assuming the problem description to be correct as I know nothing of Windowsland.

src/twisted/internet/test/test_asyncioreactor.py

twm · 2020-09-14T07:23:43Z

src/twisted/conch/test/test_knownhosts.py

@@ -1186,6 +1188,8 @@ class FakeObject(object):
    """


+
+@skipIf(not FilePath("/dev/tty").exists(), "Platform lacks /dev/tty")


This change looks odd to me. Those tests mock the open function, so how is it failing? If you have a reference to the failure you saw please file a ticket; there may be something wrong with the suite.

Do the tests still fail without this change?

twm · 2020-09-14T07:24:39Z

src/twisted/internet/test/test_asyncioreactor.py

    def test_seconds(self):
        """L{seconds} should return a plausible epoch time."""
+        if hasWindowsSelectorEventLoopPolicy:


I agree, but I don't think it's necessary to hold up the PR for this.

twm

Thanks!

rodrigc requested review from hawkowl and a team July 5, 2020 01:19

adiroiban reviewed Jul 6, 2020

View reviewed changes

azure-pipelines/windows_test_jobs.yml Outdated Show resolved Hide resolved

src/twisted/internet/test/test_asyncioreactor.py Outdated Show resolved Hide resolved

src/twisted/internet/asyncioreactor.py Outdated Show resolved Hide resolved

rodrigc force-pushed the 9766-rodrigc-asyncio-selecteventorloop branch 11 times, most recently from 4f9e98e to 6fc88a5 Compare July 7, 2020 09:46

rodrigc requested a review from adiroiban July 7, 2020 10:36

adiroiban reviewed Jul 7, 2020

View reviewed changes

src/twisted/internet/test/test_asyncioreactor.py Show resolved Hide resolved

adiroiban reviewed Jul 7, 2020

View reviewed changes

src/twisted/internet/asyncioreactor.py Outdated Show resolved Hide resolved

adiroiban reviewed Jul 7, 2020

View reviewed changes

rodrigc force-pushed the 9766-rodrigc-asyncio-selecteventorloop branch 3 times, most recently from 9b5699d to 770dd0a Compare July 9, 2020 00:09

rodrigc force-pushed the 9766-rodrigc-asyncio-selecteventorloop branch from 770dd0a to 7caf753 Compare July 20, 2020 23:00

rodrigc force-pushed the 9766-rodrigc-asyncio-selecteventorloop branch from 7caf753 to d78996d Compare August 3, 2020 18:28

rodrigc requested a review from adiroiban August 3, 2020 19:21

rodrigc mentioned this pull request Aug 31, 2020

[WIP] Require CPython 3.8 passes on Windows #1259

Closed

rodrigc force-pushed the 9766-rodrigc-asyncio-selecteventorloop branch from d78996d to 9d86143 Compare September 5, 2020 00:17

altendky requested changes Sep 5, 2020

View reviewed changes

altendky reviewed Sep 5, 2020

View reviewed changes

rodrigc force-pushed the 9766-rodrigc-asyncio-selecteventorloop branch from 9d86143 to ea1ff17 Compare September 5, 2020 02:22

twisted deleted a comment from mthuurne Sep 6, 2020

rodrigc force-pushed the 9766-rodrigc-asyncio-selecteventorloop branch from ea1ff17 to df96b4b Compare September 6, 2020 06:19

rodrigc added 5 commits September 13, 2020 18:46

twisted.internet.asyncioreactor.AsyncioSelectorReactor raises a TypeE…

0e72cc7

…rror if not passed an asyncio.SelectorEventLoop In Python 3.8+, asyncio.get_event_loop() was changed to return a asyncio.ProactorEventLoop in bpo-34687, which is incompatible with AsyncioSelectorReactor.

Add newsfragment

cbaa6ab

Do not allow CI failures on Windows Python 3.8 runs

7407fa1

Add newsfragment

abde67d

Skip DefaultAPITests on Windows where /dev/tty does not exist

a3d9e3e

rodrigc force-pushed the 9766-rodrigc-asyncio-selecteventorloop branch from df96b4b to a3d9e3e Compare September 14, 2020 01:47

twm mentioned this pull request Sep 14, 2020

[9955] Add Python 3.8 Trove classifier #1382

Merged

5 tasks

twm requested changes Sep 14, 2020

View reviewed changes

Add docstring

bf2746a

twm approved these changes Sep 14, 2020

View reviewed changes

rodrigc merged commit d8203ca into trunk Sep 14, 2020

rodrigc deleted the 9766-rodrigc-asyncio-selecteventorloop branch September 14, 2020 08:36

wRAR mentioned this pull request Feb 4, 2021

Fix and document asyncio reactor problems on Windows scrapy/scrapy#4976

Closed

BruceLee569 mentioned this pull request May 18, 2022

Unable to use on Windows? NotImplementedError of _make_subprocess_transport for SelectorEventLoop scrapy-plugins/scrapy-playwright#7

Closed

obilodeau mentioned this pull request Jun 27, 2023

PyRDP running error on Windows 10 GoSecure/pyrdp#316

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix AsyncioSelectorReactor on Windows Python 3.8+ #1338

Fix AsyncioSelectorReactor on Windows Python 3.8+ #1338

rodrigc commented Jul 5, 2020

adiroiban left a comment

adiroiban Jul 7, 2020

twm Sep 14, 2020

altendky Sep 15, 2020

adiroiban left a comment

rodrigc commented Aug 3, 2020

adiroiban commented Aug 3, 2020

rodrigc commented Aug 3, 2020

altendky commented Sep 5, 2020

altendky left a comment

altendky Sep 4, 2020

rodrigc Sep 5, 2020

altendky Sep 5, 2020

twm Sep 14, 2020

altendky Sep 22, 2020

altendky Sep 5, 2020

rodrigc Sep 5, 2020

altendky Sep 5, 2020

rodrigc commented Sep 5, 2020

twm left a comment

twm Sep 14, 2020

twm Sep 14, 2020

twm left a comment

		@@ -1186,6 +1188,8 @@ class FakeObject(object):
		"""



		@skipIf(not FilePath("/dev/tty").exists(), "Platform lacks /dev/tty")

Fix AsyncioSelectorReactor on Windows Python 3.8+ #1338

Fix AsyncioSelectorReactor on Windows Python 3.8+ #1338

Conversation

rodrigc commented Jul 5, 2020

adiroiban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban left a comment

Choose a reason for hiding this comment

rodrigc commented Aug 3, 2020

adiroiban commented Aug 3, 2020

rodrigc commented Aug 3, 2020

altendky commented Sep 5, 2020

altendky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigc commented Sep 5, 2020

twm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twm left a comment

Choose a reason for hiding this comment