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

Drop run and run_in_threadpool #710

Merged
merged 5 commits into from
Jan 6, 2020
Merged

Drop run and run_in_threadpool #710

merged 5 commits into from
Jan 6, 2020

Conversation

tomchristie
Copy link
Member

We're no longer using the backend methods run and run_in_threadpool within the httpx codebase, so this PR removes them.

We are however using them within the test cases in order to perform the server restarts.

I've changed things around so that .restart() is a standard blocking function (It may as well be, and it runs from a different thread from the server, so it won't block the server itself from shutting down.)

However, the tests that use server.restart() are currently failing in the asyncio case. (With h11 errors that the server close event is occuring at a point that it should not.) Really not clear to me why we'd see that with asyncio but not with trio. I think that are resolution here might be that restart should perform a full shutdown, and reopen the server on an entirely new thread, to keep things cleaner?

Any thoughts on this @florimondmanca?

@tomchristie tomchristie added the refactor Issues and PRs related to code refactoring label Jan 2, 2020
@florimondmanca florimondmanca self-requested a review January 3, 2020 21:30
@florimondmanca
Copy link
Member

florimondmanca commented Jan 3, 2020

Yes, I remember server restarts have already been the cause of some headaches in the past… #276 (comment)

As far as understanding the error message, it's already been discussed in #96 — basically the server abruptly closed the connection (which it did). We're making a blocking call in the main thread via time.sleep(), which seems to be OK for trio, but I'd suspect that's what causing failures on asyncio.

Actually, if we turn restart() into a coroutine again, and have it call asyncio.sleep() instead of time.sleep(), the test runs fine on asyncio — but it fails on trio, obviously, because of the asyncio-specific call.

Luckily, we have an agnostic sleep() implementation in tests/concurrency.py, so since there are no other asyncio-specific async calls in restart(), we can probably call sleep(backend, ...), and things will work on trio too?

Lemme push a commit to demonstrate the idea…

@florimondmanca
Copy link
Member

Note that these connection pool tests run faster now, because we don't have to setup/teardown a thread on each restart. 😄

@tomchristie
Copy link
Member Author

@florimondmanca Thanks, great stuff!

I've now also dropped AsyncioBackend.loop, since it's not used anywhere.

Also cleaned up the tests/concurrency.py util functions to use a more direct style than functools.single_dispatch - we don't really need to be that clever.

@tomchristie tomchristie added this to the 0.11.0 milestone Jan 6, 2020
Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

💯

@tomchristie tomchristie merged commit 6ac49da into master Jan 6, 2020
@tomchristie tomchristie deleted the drop-run branch January 6, 2020 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Issues and PRs related to code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants