-
-
Notifications
You must be signed in to change notification settings - Fork 86
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
Disconnect event is not propogated to the middleware while a request is processing #286
Comments
@speksen-kb01 yes, at the moment the network I/O in Granian doesn't allow to be notified when connection gets closed prematurely by clients. In fact, network I/O in Granian today is eager, meaning checks over connection state are run only when actually reading and writing: if you change the code to try writing back empty contents during that 10 seconds, you should be able to get the exception you're looking for. This might be changed in the future – if a valid reason to change the current behaviour exists – but it really depends on the lower library capabilities. |
Imagine that the endpoint is something that consumes too much resource and it can be called multiple times by a single client in parallel. In order not to waste resource we want to cancel as soon as we receive a disconnect event |
I see, thanks for the explanation. For the use case part, Imagine a "request chain" that depend on each other to finish, and it takes some time, for each to finish it's task. |
Just to clarify on this: this seems to be not achievable due to how Hyper is currently designed. |
I don't really know the intricacies of the granian project, so I dont know if this could help but I'd like to also add that hyper seems to drop the request task upon a disconnect. Could be related to this issue somehow: |
This issue was closed as inactive #324 but I think it relates to this. The original author probably got jack of granian and went back to something stable like gunicorn/uwsgi. I manage multiple django projects accross various organisations. One of them we stuck with granian but put connection timeouts on the postgresql server - this fixed the issue. One of them we went back to unicorn because the system admin insisted it wasn't his systems problem and didn't want to change anything if it "wasn't his fault" - not that it was but the postgresql server timeouts would have worked also. Either way, it would be good if this could be resolved. |
Definitely not related, even if you're using ASGI with Django. |
thanks for confirmation, can you please explain what the actual issue is for django though so I understand? |
Django uses the |
Thank you, will the --backpressure argument still be required for this to take effect? Would it be possible to please include an example usage in the releases/changelog for 1.4.4? |
Backpressure is still needed, the usage is the same for all the 1.4.x releases. |
Hi! This seems to be an important and problematic issue: cleanup stage won't be executed if client cancelled request. This hurts bad in my case. I want to initialize—and shutdown once done—a costly resource for each request to an SSE endpoint in Litestar application. Since cleanups are not executed after request is cancelled, this leads to a memory leak. import asyncio
import typing
import litestar
from litestar.response import ServerSentEvent
async def print_every_second() -> typing.NoReturn:
while True:
print("hi")
await asyncio.sleep(1)
async def iter_something() -> typing.AsyncIterable[str]:
async with asyncio.TaskGroup() as task_group:
task_group.create_task(print_every_second())
yield "hi"
@litestar.get("/")
async def root() -> ServerSentEvent:
return ServerSentEvent(iter_something())
app = litestar.Litestar([root]) In this example |
The example you provided doesn't seem to be related to the OP. Granian already sends disconnect events on On another side, even if it turns out to be actually related, you can find in previous discussions all the details on why this is on hold, and the overall state of this issue is quite clear IMO: contributions trying to address the actual OP issue are more than welcome. Mentioning me directly won't change any of this. |
Thanks for your answer. I didn't mean to be rude, sorry. This issue seems to be a two-piece, I have opened the issue on Litestar side as well as writing here: litestar-org/litestar#3772.
I will continue to investigate the issue with different frameworks and servers.
Yes, I understand that, I just wanted to point out that this issue may have another corner case that I highlighted. |
So, after running your MRE locally, I can say what's the issue with Granian: you're blocking the iterator which will never send the final ASGI message to the server, and thus you won't either receive the disconnect event (which makes sense, the client is still connected waiting for more body). Changing your snippet with this: async def iter_something() -> typing.AsyncIterable[str]:
async with asyncio.TaskGroup() as task_group:
task = task_group.create_task(print_every_second())
yield "hi"
task.cancel() makes everything work as intended. |
This snippet basically means this: yield once, and proactively cancel the task. More realistic example would be this: import asyncio
import typing
import litestar
from litestar.response import ServerSentEvent
async def print_every_second() -> typing.NoReturn:
while True:
print("hi")
await asyncio.sleep(1)
async def iter_something() -> typing.AsyncIterable[str]:
async with asyncio.TaskGroup() as task_group:
task_group.create_task(print_every_second())
while True:
yield "hi"
await asyncio.sleep(1)
@litestar.get("/")
async def root() -> ServerSentEvent:
return ServerSentEvent(iter_something())
app = litestar.Litestar([root]) SSE route generates data forever—and won't be interrupted when client cancels the request. Uvicorn—on other hand—would send CancelledError into the async generator—task group would understand it and cancel the task by itself. |
This function will exit without intervention by CancelledError: whether client cancels the request or not |
No it doesn't. Tested that snippet locally and works as expected, as soon as I close the request on the client-side Granian logs that:
and sends the disconnect event to the |
Sure: #412 |
Hello,
We were working on an HttpDisconnectMiddleware, which would cancel the ongoing tasks when user cancels the requests.
We were using this same middleware with other servers like uvicorn and hypercorn, which would trigger the exception upon cancellation, but in the case of granian, it seems like the disconnect event is queued and not sent to the middleware, and only sent to it after the task finishes.
I've created a repo for showcasing the effect,
https://github.com/speksen-kb01/fastapi-disconnect-example
Is there a reason why we couldn't get the event while a request is being processed? If so, what do you think it is related to and is it fixable by changing the middleware to some extent?
Funding
The text was updated successfully, but these errors were encountered: