(WIP) remove ClientHello from the client creation path

[erikbern]

This should reduce the e2e latency of running ephemeral apps by about 100-250ms. It removes two server round trips sitting in the critical path (one locally, one in the container). It also simplifies the RPC protocol a bit, which seems useful (like if we want to add support for other languages).

Instead of relying on ClientHello to catch various connection issues, we now rely on any request.

TODO: This makes it slightly harder to print helpful things like "your client version is too old", which is I think the only real drawback here. I'll think about ways we can do this using other RPCs instead. However this might require moving a bunch of server code from the ClientHello method into the middleware.

[erikbern]

This relied on a bit of a questionable behavior where a failing ClientHello would cause the singleton to not register. Now that we're removing ClientHello from Client.from_env, we don't catch the connection error in a way that resets the singleton, which means we leak it into the rest of the function.

I think this is a bit silly – I think it should be safe to assume that Client.from_env always creates one single unified client based on the environment.

We have an auto-fixture to reset the client singleton so splitting it in to two tests resolve this state leakage.

[freider]

Super strange test failure. It seems like there is some side-effect to running hello() which then affects the later calls to UnaryStreamMethod.open() using the same client.

It's probably not the end of the world to skip these assertions, since they are mostly for making sure we warn if we accidentally use the anti-pattern of calling our API stubs from outside of the synchronicity event loop (since that would break the cancellation context which is event-loop specific). In a way the exception makes sense, and I'm not actually 100% sure how the test passed before this... Will look a bit more

[freider]

Ok, the issue stems from grpclib.Channel being lazy and that it doesn't actually establishing an h2 connection when you create the channel but does so lazily the first time it's used with send_request.

Now that .hello() isn't being called when we open the client, that means the h2 connection is bound to the main event loop instead of the synchronicity event loop, and something in the code tries to interact with it from the latter (which also seems like a bug though, since the test tries to explicitly use the non-synchronicity event loop)

[freider]

Ahhh, grpclib.Channel registers the running event loop on creation (which in our case is the synchronicity loop), and then references it when it later tries to connect (which now happens outside of the event loop).

All in all pretty bad that we even allow usage of the client's API stub across event loops - considering the amount of foot guns it exposes I think we might want to just raise an exception when that happens and expect all of the SDK + tests to only use the Client + api stub from a single event loop.