sysid / sse-starlette

BSD 3-Clause "New" or "Revised" License
505 stars 36 forks source link

Question: How to handle client disconnection? #7

Closed firatkucuk closed 3 years ago

firatkucuk commented 3 years ago

The client can unexpectedly disconnect but I couldn't find an easy way of doing that.

paxcodes commented 3 years ago

I have the same question.

I already am checking request.is_disconnected() but notice when client connection is "lost", it still sends PING events (see backend_1 | TRACE: 172.24.0.3:39602 - Connection lost that happens just before the last 2 pings:

uvicorn trace logs ``` backend_1 | INFO: 172.24.0.3:39602 - "GET /lobby/PAXY6 HTTP/1.1" 200 OK backend_1 | DEBUG: chunk: data: Pax backend_1 | backend_1 | backend_1 | TRACE: 172.24.0.3:39602 - ASGI [3] Send {'type': 'http.response.body', 'body': '<13 bytes>', 'more_body': True} backend_1 | TRACE: 172.24.0.3:39602 - ASGI [3] Receive {'type': 'http.request', 'body': '<0 bytes>', 'more_body': False} backend_1 | DEBUG: ping: event: ping backend_1 | data: 2020-10-06 00:42:29.957986 backend_1 | backend_1 | backend_1 | TRACE: 172.24.0.3:39602 - ASGI [3] Send {'type': 'http.response.body', 'body': '<49 bytes>', 'more_body': True} backend_1 | DEBUG: ping: event: ping backend_1 | data: 2020-10-06 00:42:44.959141 backend_1 | backend_1 | backend_1 | TRACE: 172.24.0.3:39602 - ASGI [3] Send {'type': 'http.response.body', 'body': '<49 bytes>', 'more_body': True} backend_1 | TRACE: 172.24.0.3:39602 - Connection lost backend_1 | DEBUG: ping: event: ping backend_1 | data: 2020-10-06 00:42:59.938124 backend_1 | backend_1 | backend_1 | TRACE: 172.24.0.3:39602 - ASGI [3] Send {'type': 'http.response.body', 'body': '<49 bytes>', 'more_body': True} backend_1 | DEBUG: ping: event: ping backend_1 | data: 2020-10-06 00:43:14.938602 backend_1 | backend_1 | backend_1 | TRACE: 172.24.0.3:39602 - ASGI [3] Send {'type': 'http.response.body', 'body': '<49 bytes>', 'more_body': True} ```

I wonder if these issues are at all relevant:

https://github.com/encode/uvicorn/issues/388 Discussion starting in this comment

paxcodes commented 3 years ago

The more I read this discussion and comparing it to the current code --- the more I think this is a bug?

What do you think @sysid ?

sysid commented 3 years ago

Thanks @paxcodes for your analysis, much appreciated. I tend to agree but I currently I cannot dig deeper into it. If you have an idea for a quick fix, please let me know.

I will try to find time asap, though.

PS: Do you think this might be a way to follow? https://github.com/encode/starlette/blob/b3271a53b0b5c98ef3117dc0b82bd7f700fb44a8/starlette/responses.py#L216

paxcodes commented 3 years ago

Exactly. I was looking at that.

Is there a reason why we are not simply inheriting from StreamingResponse similar to what this person did? IIUC, if we inherit StreamingResponse, this scenario will be taken care of.

sysid commented 3 years ago

Done. Pretty much followed your proposition. Thanks again! #10

paxcodes commented 3 years ago

Thanks so much, @sysid for responding so quickly. It does fix my use case. Any chance you'll publish a release soon?

paxcodes commented 3 years ago

By the way, I realized yesterday that the /endless endpoint works perfectly even before this PR was merged. That is, the server stops streaming once the client disconnects.

I was simply not using request.is_disconnected() properly:

async streamMessages():
        while True:
            disconnected = await req.is_disconnected()
            if disconnected:
                break
            async with broadcast.subscribe(channel=room.code) as subscriber:
                async for event in subscriber:
                    msg = event.message
                    yield msg

return EventSourceResponse(streamMessages())

But the merge was still beneficial since I can now do this without having to worry about client disconnection (since this package takes care of it already):

async streamMessages():
    async with broadcast.subscribe(channel=room.code) as subscriber:
        async for event in subscriber:
            msg = event.message
            yield msg

return EventSourceResponse(streamMessages())
vaibhavmule commented 3 years ago

@paxcodes if you don't mind, can you share code broadcast, I am trying to send a notification when user creates or updates comments. the code you mentioned above looks good.

UrbanSwati commented 3 years ago

@paxcodes if you don't mind, can you share code broadcast, I am trying to send a notification when user creates or updates comments. the code you mentioned above looks good.

I'm assuming @paxcodes is using redis or similar package

https://redislabs.com/ebook/part-2-core-concepts/chapter-3-commands-in-redis/3-6-publishsubscribe/

paxcodes commented 3 years ago

Thanks for chiming in @UrbanSwati -- I emailed vaibhavmule as I don't want to hi-jack this issue's thread.