sysid / sse-starlette

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

Heartbeat event? #51

Closed nuance1979 closed 1 year ago

nuance1979 commented 1 year ago

I have a FastAPI server with sse-starlette for SSE. Everything works smoothly except occasionally, my client would receive an event with data looking like a timestamp:

data= 2023-04-07 23:36:05.744043
data= 2023-04-07 23:36:20.893051

I checked my server code and can be 100% sure that my server never sent events with these data. What could be the reason? How can I filter/ignore them?

paxcodes commented 1 year ago

Can you give a minimum, reproducible example? I can try to help.

===

EDIT: Sorry, I remember now. Yes, it's some sort of heartbeat to keep legacy proxy servers from dropping the HTTP connection. You can ignore them by filtering out any message/event whose type is a ping

nuance1979 commented 1 year ago

Thanks @paxcodes ! I believe I found what you were referring to:

https://github.com/sysid/sse-starlette/blob/30ef55c08a8b1512b625752be136a1ea67df6030/sse_starlette/sse.py#L275

However, what's strange is that I've already filtering on event.type (my server sends event.type='event'):

async start(controller) {
        const onParse = (event: ParsedEvent | ReconnectInterval) => {
          if (event.type === 'event') {
            const data = event.data;

But I still got the ping event. What do you think might be happening here?

paxcodes commented 1 year ago

Do I understand your situation correctly?

// client code 

if (event.type === 'event') {
    const data = event.data;
    // !!! data is some sort of timestamp
}

If that is the case, that is strange. How are the events parsed on the client side? Who is providing that event parameter / ParsedEvent object? An idea is that the client is not parsing the event-stream correctly and setting an incorrect event type.

That assumes that sse-startlette has the expected event-stream text that looks like \r\nevent: ping\r\n

So yes, I would check:

a) What your server is actually sending b) How your client is parsing what it receives from the server

Using a browser as a client might help you troubleshoot as well because you can have visibility to what the server is sending through the Network tab,

image

michaelroyzen commented 1 year ago

I'm getting the same issue. Can there be an option for just disabling the heartbeat? That would seem logical to me. It's incredibly annoying (and bad design) to always have to parse it out on the client.

EDIT: The best solution I've found is to set the ping to an arbitrarily high number, but there should be an option to disable IMO.

paxcodes commented 1 year ago

I'll work on a PR to set the ping as a comment instead so clients don't have to deal with it.

sysid commented 1 year ago

@paxcodes , thank you again for taking initiative!!

I updated the example comment_as_ping.py to show that it is already possible to send the ping event as comment. Hope this helps.

paxcodes commented 1 year ago

@sysid Would you merge a PR that sends pings as comments by default? Given the 3 users who commented about them already, I wonder if it's worth doing so that clients don't have to deal with ping events out-of-the-box.