added missing connection close in splitter

[mackuba]

I've been trying to track down an issue where in my firehose client (in Ruby) the websocket connection sometimes gets stuck, meaning that at some point events stop flowing, but the connection doesn't disconnect, so the client doesn't notice immediately that something is wrong. I've eventually added a "heartbeat" feature to my library Skyfall, where it has a timer that checks the time since the last event, and forces a reconnect if there haven't been any events for a while. This has been happening very occasionally until autumn last year, but started getting much more common since November. I worked around this by making the timer limits much lower.

But I started digging into this, and I got the feeling that the problem must be in the server's code, not on my side. I can trigger this consistently almost every time, when I start catching up from a bit more behind, like a few hours behind.

So I looked through the indigo code, ended up setting a test relay/splitter on a spare VPS, and I finally tracked this down to… one missing line that disconnects the connection :) It looks like bgs.go and splitter.go share a lot of the code, and specifically the beginning of this EventsHandler method is identical, except this one line (https://github.com/bluesky-social/indigo/blob/main/bgs/bgs.go#L548) that somehow got lost on the way.

Without this line, when the EventManager in events.go kills the consumer, it's removed from all the lists and the server forgets about it, but the connection itself is not disconnected, so the client is left hanging there, not knowing what's going on.

This is related to a second issue I'll file a separate ticket for, which is that with the right conditions (which aren't hard to meet), a catching up consumer gets dropped in this in-between phase when catching up what was added in the meantime, before switching to the live stream. So with this fix, the consumer will at least be disconnected immediately, which is better because then you aren't wasting time and falling behind again while being stuck.

EDIT: issue added here #975

[bnewbold]

Looks good to me, thanks!

This would, presumably, impact rainbow, but not bigsky.

bgs/bgs.go and cmd/supercollider/main.go both include defer statements like this; pds/server.go does not (note that the PDS code isn't really used).

[mackuba]

Yeah, which is I think it started happening much more in mid November when you've switched the relay implementation

[brianolson]

LGTM, we probably usually get away with the client closing but we shouldn't rely on that and when we decide to close we should close

[mackuba]

@brianolson yeah, but in this case the problem is that the client doesn't know that they need to close - they get no indication that something is wrong other than the fact that events aren't coming over the stream (which isn't always an unrecoverable problem since it could be a glitch in the network connection that goes away after a moment)