vmagamedov/grpclib

Allow creating a `Channel` outside of an async context

FHTMitchell opened this issue · 5 comments

There is no reason to demand that a Channel is constructed in an async context but currently that is (subtly) the case.

class Channel:
    ...
    def __init__(...):
        ...
        if loop:
            warnings.warn("The loop argument is deprecated and scheduled "
                          "for removal in grpclib 0.5",
                          DeprecationWarning, stacklevel=2)
        self._loop = loop or asyncio.get_event_loop()

This can be found here. That call to asyncio.get_event_loop is deprecated outside of an async runtime. See the python docs.

Now this will be fixed by 0.5 where you promise to drop the support for the loop param and will presumably just always grab the asyncio event loop at the call site (if even needed), but we don't have to wait for a breaking release. I propose we do this in the meantime

class Channel:
     def __init__(...):
         if loop:
              warnings.warn(...)
         self._running_loop = loop
         ...
    @property
    def _loop(self):
         # This should only be called from within an async runtime!
         return self._running_loop or asyncio.get_running_loop()

(I did consider setting the loop also in the async def __aenter__ method (if it's unset) but I doubt it gives any performance benefit).

This prevents the kind of errors as seen here:

In [2]: %autoawait
IPython autoawait is `on`, and set to use `asyncio`

In [3]: import vol.vol_grpc

In [4]: import grpclib.client

In [5]: channel = grpclib.client.Channel('localhost', 50003)  # perfectly valid to construct object outside of async runtime

In [6]: async with channel as ch:
    ...:    client = vol.vol_grpc.VolServiceStub(ch)
    ...:    res = await client.RunVolPipeline(vol.vol_grpc.vol.PyvolRequest())
...
RuntimeError: Task <Task pending coro=<InteractiveShell.run_cell_async() running at .../IPython/core/interactiveshell.py:3257> cb=[_run_until_complete_cb() at .../asyncio/base_events.py:157]> got Future <Future pending cb=[_chain_future.<locals>._call_check_cancel() at .../asyncio/futures.py:351]> attached to a different loop.

This is because the loop when we created the Channel is replaced when starting a new loop (in this case automatically by IPython but the result is the same if we explicitly call asyncio.run). The code suggested before would allow this to work.

I'm happy to do the PR, it's basically just what's above :)

Hello, I totally agree that this can be fixed before 0.5 release and without breaking backward compatibility.

I'm only asking to avoid creation of extra "properties" for simplicity. I suggest you to lazily initialise this property maybe inside async def _create_connection(self): method. Users may not use __aenter__ context manager protocol, it is optional for a Channel.

Looking forward to review and merge your PR.

Regarding this issue, I suspect there's another issue related. When the gRPC client is run within an asyncio context that is later closed, invoking gRPC methods on that client sends the request to the server, but then it just hangs - ignoring the response.
I understand that once the client is created, the relevant loop instance is pinned to it, but it's not apparent from the constructor API. So, as a developer, I'm unaware that there's a state that I should manage that grpclib depends on. I'd expect either the client re-fetches the loop on new requests, or raises an exception upon request.