permitio / fastapi_websocket_rpc

⚡ FASTAPI Websocket RPC- A fast and durable bidirectional JSON RPC channel over Websockets.
https://permit.io
MIT License
216 stars 25 forks source link

Long-running RPC handlers block other RPC requests #48

Open sersorrel opened 1 month ago

sersorrel commented 1 month ago

Say you have the following handler:

    async def takes_a_moment(self) -> str:
        await asyncio.sleep(1)
        return "hello world"

I adapted the test_recursive_rpc_calls test to see what would happen if I called this several times simultaneously (using asyncio.create_task to schedule all the calls for execution at once, rather than waiting for them to complete in order). I expected that while the handler is asyncio.sleeping, other handlers would be free to run, so regardless of how many times I call the handler, it shouldn't take much more than a second to complete (since that's the behaviour I would see if I were calling it like a regular async function, rather than via the RPC protocol):

@pytest.mark.asyncio
async def test_simultaneous_calls(server):
    async with WebSocketRpcClient(uri, RpcUtilityMethods(), default_response_timeout=10) as client:
        start = time.monotonic()
        t1 = asyncio.create_task(client.other.takes_a_moment())
        t2 = asyncio.create_task(client.other.takes_a_moment())
        t3 = asyncio.create_task(client.other.takes_a_moment())
        t4 = asyncio.create_task(client.other.takes_a_moment())
        t5 = asyncio.create_task(client.other.takes_a_moment())
        await asyncio.gather(t1, t2, t3, t4, t5)
        end = time.monotonic()
        # each request takes 1 second, and they should happen simultaneously,
        # so in total they should take only a little more than a second
        assert end - start < 2

However, this test fails:

FAILED tests/advanced_rpc_test.py::test_simultaneous_calls - assert (9187774.350943424 - 9187769.341026856) < 2

Rather than running simultaneously, the handlers are running one after the other – so the test takes five full seconds to complete...

$ echo "9187774.350943424 - 9187769.341026856" | bc
5.009916568

Is it expected behaviour that only one RPC handler can be executing at any given time?

orweis commented 1 month ago

Hi thanks for reaching out :)

Reading the docs , or the code (e.g. definition of .other, definition of call, definition of async_call) will show you that .other and call are synchronous while async_call is async.

You'd probably have better results using channel.async_call() and gathering on the wait function of the promises.

sersorrel commented 1 month ago

Thanks for the pointers. Looking more closely, I'm not actually sure I can achieve the behaviour I want as it stands, since WebsocketRPCEndpoint fundamentally waits for each call to complete before it starts listening for the next call:

https://github.com/permitio/fastapi_websocket_rpc/blob/307c14980663ad18cc4f4a73eb98b858a87704d2/fastapi_websocket_rpc/websocket_rpc_endpoint.py#L90-L92

I suppose I could use a callback style, like call_me_back does:

https://github.com/permitio/fastapi_websocket_rpc/blob/307c14980663ad18cc4f4a73eb98b858a87704d2/fastapi_websocket_rpc/rpc_methods.py#L82-L90

but that leaks the result of create_task, which means the async_call task could be garbage-collected at any moment, and anyway if I wanted to handle the callbacks myself I could just run a regular HTTP server on both ends and requests.get() between them :)

I will think about ways to improve this.

orweis commented 1 month ago

fundamentally waits for each call to complete before it starts listening for the next call

If your rpc_method being called is async (meaning spins off a task to do it's work) then that's a none-issue.

but that leaks the result of create_task

That's just a testing utility method Use async_call directly and save the created task (for example in a class member -this would work very well with a context manager class you can have for your service).