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

Long-running RPC handlers block other RPC requests #48

Open
sersorrel opened this issue Sep 24, 2024 · 3 comments · May be fixed by #49
Open

Long-running RPC handlers block other RPC requests #48

sersorrel opened this issue Sep 24, 2024 · 3 comments · May be fixed by #49

Comments

@sersorrel
Copy link

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
Copy link
Contributor

orweis commented Sep 25, 2024

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
Copy link
Author

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:

while True:
data = await simple_websocket.recv()
await channel.on_message(data)

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

async def call_me_back(self, method_name="", args={}) -> str:
if self.channel is not None:
# generate a uid we can use to track this request
call_id = gen_uid()
# Call async - without waiting to avoid locking the event_loop
asyncio.create_task(self.channel.async_call(
method_name, args=args, call_id=call_id))
# return the id- which can be used to check the response once it's received
return call_id

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
Copy link
Contributor

orweis commented Sep 25, 2024

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).

@sersorrel sersorrel linked a pull request Sep 25, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants