Skip to content

Conversation

@graingert
Copy link
Contributor

@graingert graingert commented Jun 4, 2021

in 3.10 running asyncio.get_event_loop() when asyncio.get_running_loop() would fail is deprecated https://docs.python.org/3.10/library/asyncio-eventloop.html#asyncio.get_event_loop

@graingert graingert force-pushed the avoid-deprecated-get-event-loop branch from 9092c9e to 73dc7e9 Compare June 4, 2021 18:58
@graingert graingert force-pushed the avoid-deprecated-get-event-loop branch from 73dc7e9 to a51908a Compare June 4, 2021 19:00
@euri10
Copy link
Contributor

euri10 commented Jun 7, 2021

I've played a little with 3.10 and the test suite, it seems to me those changes makes sense, however there is a bunch of tests that will fail on 3.10 mainly on the way we setup the loop and get access to it (and on ssl protocol not set but it's unrelated to that and I think the fix is simpler), and unfortunately the PR does not solve it.
I'm not sure if the github workflow already has an image for 3.10 but what about adding it to the workflow and see how/if 3.10 can be added ?

@Kludex
Copy link
Owner

Kludex commented Jun 7, 2021

I had to cancel the workflow for both Windows and Ubuntu 3.10, jfyk.

EDIT: Due to infinite loop... It was consuming our quota 😗

@graingert graingert changed the title avoid deprecated asyncio.get_event_loop test under py3.10 Jun 8, 2021
@graingert
Copy link
Contributor Author

PR for httpx here: encode/httpx#1671

Copy link
Contributor

@euri10 euri10 left a comment

Choose a reason for hiding this comment

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

Looks very good to me, approved and many thanks for that @graingert !

I just have one minor question / issue since I'm no semver pro, but would the ssl min default that is bumped from PROTOCOL_SSLv23 to PROTOCOL_TLS_SERVER qualify for a minor version bump or we good with just a patch ?

expected_loop = "asyncio" if uvloop is None else "uvloop"
assert type(policy).__module__.startswith(expected_loop)
loop = asyncio.get_event_loop()
loop.close()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@euri10 what's this for asyncio.get_event_loop() can't be called here

@graingert
Copy link
Contributor Author

The tests hang due to:
python-websockets/websockets#1021 (comment)

@graingert graingert force-pushed the avoid-deprecated-get-event-loop branch from a0c988a to 011ae96 Compare July 25, 2021 22:12
@graingert graingert force-pushed the avoid-deprecated-get-event-loop branch from 4701cbe to b35ac26 Compare July 25, 2021 22:22
@graingert
Copy link
Contributor Author

@euri10 @Kludex anyio is now always available in the test deps, what do you think about replacing pytest-asyncio with anyio to save a dep?

@graingert graingert requested a review from euri10 July 26, 2021 07:05
@graingert graingert requested a review from Kludex July 26, 2021 13:26
@graingert graingert removed the hold Don't merge yet label Jul 30, 2021
@graingert graingert merged commit c87c783 into Kludex:master Jul 30, 2021
@graingert graingert deleted the avoid-deprecated-get-event-loop branch July 30, 2021 11:56
@Kludex Kludex mentioned this pull request Aug 11, 2021
1 task
Comment on lines -17 to -19
else:
loop = asyncio.new_event_loop()
asyncio.set_event_loop(loop)
Copy link
Owner

Choose a reason for hiding this comment

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

This is the cause of the httpx tests failing. @tomchristie

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @Kludex - No idea how you tracked that down, but... 👍

Kludex added a commit that referenced this pull request Nov 17, 2021
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Kludex added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
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.

4 participants