Add trio concurrency backend

[florimondmanca]

Fixes #120

Current status: we're blocked by the connection pool implicitly relying on asyncio polling sockets in the background to tell if they're still readable. See discussion around #276 (comment).

Most of the notes below are outdated.

---

Getting the tests to pass on this one ended up having to tackle more tricky issues than I initially thought. (There's still start_tls() to figure out, but that will be for a future PR.)

So, a few notes, from the most straight-forward to the most obscure:

• I took the liberty to refactor HTTP2Dispatcher.initiate_connection() into an async function; it allowed to remove the need for write_no_block() on the concurrency backend (which trio does not have an obvious equivalent for). Things seem to be okay, but if I missed any reasons why it was not async in the first place, let me know! cc @tomchristie
• I had to make connections context-managed in HTTPS tests in test_connections.py — otherwise they'd stall when using Trio, probably because the underlying TCP stream would be waiting to be closed. I'm not 100% sure I understand the origin of this, though. Anyway, I ended up converting all those tests to use async with for consistency.
• For async tests, I had to figure out a way to allow an alternative I/O library (trio) to coexist with asyncio. The solution I went for is spawning backend.run() in the asyncio threadpool. If the code needs better/more comments there, let me know.
• For the connection pool tests, I had to figure out how to restart the uvicorn server from an async test that could be running on something else than asyncio. I added a detailed comment in the code about the solution I've come up with — let me know if it's clear enough.

If it can help with reviewing, I'd be happy to pytest.skip() some tests in this PR and then submit solutions for the tricky parts in separate PRs that resolve those skips. Let me know how you'd like to handle this. :-)

[sethmlarson]

🎉 Very exciting!

[yeraydiazdiaz]

This is great, I now have an excuse to look into Trio 😄

Left some comments and a question.

[sethmlarson]

Can you verify if Trio properly doesn't sent SNI when it receives an IP address? If it does we need to filter that out within our backend implementation. If Trio has the proper behavior we can close #278 and add some documentation for future users / backend implementations.

[sethmlarson]

Unrelated to this PR: but can we change all instances of https://127.0.0.1:8000 into using a property of the server like a URL instance perhaps? Then we can change that whenever we need to or easily test different paths via server.url.copy_with(path="/echo")

[florimondmanca]

Yup, good idea @sethmlarson. As you mentioned this can (should?) be implemented in a separate PR. :-)

[florimondmanca]

I'm encountering a bug when performing a real-life HTTP/2 + TLS request with the TrioBackend:

# example.py
import trio
import httpx
from httpx.concurrency.trio import TrioBackend


async def main(url):
    async with httpx.AsyncClient(backend=TrioBackend()) as client:
        await client.get(url)


trio.run(main, "https://example.com")

$ python example.py
Traceback (most recent call last):
  File "example.py", line 11, in <module>
    trio.run(main, "https://example.com")
  File "/Users/florimond/Developer/python-projects/httpx/venv/lib/python3.7/site-packages/trio/_core/_run.py", line 1783, in run
    raise runner.main_task_outcome.error
  File "example.py", line 8, in main
    await client.get(url)
  File "/Users/florimond/Developer/python-projects/httpx/httpx/client.py", line 363, in get
    trust_env=trust_env,
  File "/Users/florimond/Developer/python-projects/httpx/httpx/client.py", line 610, in request
    trust_env=trust_env,
  File "/Users/florimond/Developer/python-projects/httpx/httpx/client.py", line 181, in send
    allow_redirects=allow_redirects,
  File "/Users/florimond/Developer/python-projects/httpx/httpx/client.py", line 218, in send_handling_redirects
    request, verify=verify, cert=cert, timeout=timeout
  File "/Users/florimond/Developer/python-projects/httpx/httpx/dispatch/connection_pool.py", line 120, in send
    raise exc
  File "/Users/florimond/Developer/python-projects/httpx/httpx/dispatch/connection_pool.py", line 115, in send
    request, verify=verify, cert=cert, timeout=timeout
  File "/Users/florimond/Developer/python-projects/httpx/httpx/dispatch/connection.py", line 57, in send
    response = await self.h2_connection.send(request, timeout=timeout)
  File "/Users/florimond/Developer/python-projects/httpx/httpx/dispatch/http2.py", line 45, in send
    status_code, headers = await self.receive_response(stream_id, timeout)
  File "/Users/florimond/Developer/python-projects/httpx/httpx/dispatch/http2.py", line 119, in receive_response
    event = await self.receive_event(stream_id, timeout)
  File "/Users/florimond/Developer/python-projects/httpx/httpx/dispatch/http2.py", line 158, in receive_event
    await self.stream.write(data_to_send, timeout)
  File "/Users/florimond/Developer/python-projects/httpx/httpx/concurrency/trio.py", line 86, in write
    await self.stream.send_all(data)
  File "/Users/florimond/Developer/python-projects/httpx/venv/lib/python3.7/site-packages/trio/_ssl.py", line 684, in send_all
    with self._outer_send_conflict_detector:
  File "/Users/florimond/Developer/python-projects/httpx/venv/lib/python3.7/site-packages/trio/_util.py", line 122, in __enter__
    raise trio.BusyResourceError(self._msg)
trio.BusyResourceError: another task is currently sending data on this SSLStream

Everything is fine if I force http_versions=["HTTP/1.1"] on AsyncClient, so HTTP/2 is indeed failing here.

The place where this error is raised alternates between .receive_event() (as above) and .end_stream(). They run in parallel via the background manager, respectively as part of receiving the response (which also requires sending some data back to the server) and sending request data.

But, quoting the description of BusyResourceError in the Trio docs:

If two tasks try to send data through the same socket at the same time, Trio will raise BusyResourceError instead of letting the data get scrambled.

And there's no way around it — probably a way for Trio to tell us to do things differently.

…This is where the "read/write for a while" approach that @tomchristie mentioned here as an alternative to BackgroundMananger starts to make sense to me. I don't know exactly how it would look if implemented, but it would probably ensure that we're not writing to the stream at the same time from two different places.

Recap: from my understanding, right now the Trio backend can't be made compatible with HTTP/2, and we need to rework the sending/reception of request/response data to unblock this.

Since HTTP/1.1 is working though, what would be everyone's thoughts on transparently disabling HTTP/2 on trio for now temporarily, so we can get Trio + HTTP/1.1 in, and add HTTP/2 support later?

(I managed to force usage of HTTP/1.1 by executing:

ssl_context.set_alpn_protocols(["h1"])
ssl_context.set_npn_protocols(["h1"])

before opening the SSLStream in TrioBackend.connect().)

[sethmlarson]

@florimondmanca That makes me think that our asyncio implementation might have the same issue? Maybe we need to revisit the concurrency issue for HTTP/2 @tomchristie?

[florimondmanca]

@sethmlarson Yes, if asyncio does not have guards in place it’s very possible that we’re writing to the stream writer at the same time as well, potentially sending corrupted data (although up to now it doesn’t seem to have surfaced?).

[florimondmanca]

I pushed a commit demonstrating reversing the way we do things: serving the server in a separate thread, which allows us to do all async stuff in the main thread and a "native" async environment.

(I've made pytest share the server during the whole test session, so as a benefit tests are now super fast since we don't need to set it up/tear it down on every test. Also we don't need to run sync tests in the threadpool anymore.)

The commit also turns the server restart into a no-op to prove it has no effect on the tests.

There's still a failing tests for the case where an exception is raised within the ASGI app. The failure might come from how we're handling that case — for now we don't feed the app exception back into the background manager in response.on_close(). I tried fiddling around with it but it's tricky — especially because the trio traceback doesn't highlight any specific part of our code. Still, the comment we put there stating that the background manager is a weak design solution is definitely relevant.

[tomchristie]

it allowed to remove the need for write_no_block() on the concurrency backend (which trio does not have an obvious equivalent for). Things seem to be okay, but if I missed any reasons why it was not async in the first place, let me know!

So the reasoning there was because initiate_connection needs to be an atomic operation, that's performed exactly once on the first stream, before any other streams start sending messages.

If we can ensure that it's not async then we know that'll be the case since we can't task switch around it (an alternative would be to introduce a lock around it.)

[tomchristie]

I pushed a commit demonstrating reversing the way we do things: serving the server in a separate thread, which allows us to do all async stuff in the main thread and a "native" async environment.

Would there be any sense in creating a pull request that only does that, without the rest of this, so that we can get that into the test suite and confirm it, independently of the trio work?

[florimondmanca]

So, I think I got confused about what a readable socket means — which means the implementation originally pushed last week was correct. I added a bunch of code comments along with the last commit to clarify the asyncio and trio implementations of .is_connection_dropped().

If all looks well, I'd love a new review from @sethmlarson, and @tomchristie's approval for merging this. (I know there were past discussions about whether trio support should land in core, or be released as a third-party package. My position is that we should merge this in core first, and then possibly extract it into a separate package once we have enough live experience.)

[sethmlarson]

This looks great! 🎉

[sethmlarson]

Now that 0.7.3 is out you ready to merge this?

[florimondmanca]

Yup, if this sounds good to you then let's merge. I'll add docs in a subsequent PR. :)

[sethmlarson]

It's happening!!! 💥

[sethmlarson]

What an amazing mountain of work, thank you @florimondmanca and @tomchristie, @pgjones, @yeraydiazdiaz, @pquentin, and @njsmith for reviewing! 🎉

[florimondmanca]

Thanks to everyone who reviewed and commented! 🌟