Skip to content

Conversation

provinzkraut
Copy link
Member

@provinzkraut provinzkraut commented Feb 2, 2025

poc for listening for early client disconnects and cancelling handler functions / responses in progress.


I currently consider this not viable, since it imposes a 50% performance penalty when looking at throughput of a simple plaintext benchmark. This is due to the overhead introduced by running the response cycle in an anyio.TaskGroup, which is considerable. There is a pure asyncio solution, which is a bit more performant, but overhead is still in the same ballpark.


This also surfaces some flaws in how we're handling static files, which is what's causing most of the test failures, and would need to be addressed.


Open questions

  • Should background tasks be cancelled?
  • Should background tasks run if the response hasn't finished due to the client disconnecting?

@github-actions github-actions bot added the area/dependencies This PR involves changes to the dependencies label Feb 2, 2025
Comment on lines +128 to +141
async with anyio.create_task_group() as tg:
tg.start_soon(
partial(
self._handle_response_cycle,
scope=scope,
send=send,
receive=receive,
request=request,
route_handler=route_handler,
parameter_model=parameter_model,
cancel_scope=tg.cancel_scope,
),
)
tg.start_soon(self._listen_for_disconnect, request, tg.cancel_scope)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a more performant, but we can't really use it for reasons of anyio/trio compatibility.

Suggested change
async with anyio.create_task_group() as tg:
tg.start_soon(
partial(
self._handle_response_cycle,
scope=scope,
send=send,
receive=receive,
request=request,
route_handler=route_handler,
parameter_model=parameter_model,
cancel_scope=tg.cancel_scope,
),
)
tg.start_soon(self._listen_for_disconnect, request, tg.cancel_scope)
tasks = [
asyncio.create_task(
self._handle_response_cycle(
scope=scope,
send=send,
receive=receive,
request=request,
route_handler=route_handler,
parameter_model=parameter_model,
)
),
asyncio.create_task(request._listen_for_disconnect()),
]
done, pending = await asyncio.wait(tasks, return_when=asyncio.FIRST_COMPLETED)
done.pop().result()
if done:
done.pop().result()
if pending:
pending = pending.pop()
pending.cancel()
await pending
pending.result()

@provinzkraut
Copy link
Member Author

What I could see is maybe having this be an opt-in feature. That would introduce additional complexity, but leave it up to the user to decide whether the overhead is worth it. I'd suspect though, that in most cases, a simple timeout on the request would suffice

@FHU-yezi
Copy link

FHU-yezi commented Feb 3, 2025

What I could see is maybe having this be an opt-in feature. That would introduce additional complexity, but leave it up to the user to decide whether the overhead is worth it. I'd suspect though, that in most cases, a simple timeout on the request would suffice.

Make sense. For most of use cases, the action is not so heavy that we should cancel the handler when use disconnected. User disconnected should be considered as an exception, not a common situation.

The idempotency of API should be handled by backend logic, according to the HTTP method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants