philippseith / signalr

SignalR server and client in go
MIT License
131 stars 39 forks source link

Client unable to know when server stops sending events #171

Closed roks0n closed 1 year ago

roks0n commented 1 year ago

I'm connecting to a signalr server (official dot net implementation) and I've noticed that the client doesn't get notified when the server stops receiving any events (even ping).

I'd expect if the client has a timeout set to 30 seconds and if server doesn't send any event or response to a ping to break out of the loop with a timeout error: https://github.com/philippseith/signalr/blob/master/loop.go#L112

Based on my limited understanding of Go that code doesn't appear to ever get hit given Timeout is recommended to be double the value of KeepAlive?

How to reproduce:

  1. Establish a connection to an external signalr server.
  2. Then disconnect from the internet.
  3. You will see the client keeps pinging the server (for 2min+) and the connection never timeouts. Connection state remains connected.
  4. Connect to internet.
  5. You will see the client continues pinging and the server never responds. Checking connection state it keep saying it's connected.

I wanted to make a workaround by setting a receiver for ping events for when the server sends them and then disconnecting if the client when the time difference would be greater than Timeout, but it seems that interface doesn't expose a method for this?

philippseith commented 1 year ago

Did you use SSE or websockets?

roks0n commented 1 year ago

The client establishes a websocket connection with the 3rd party signalr server. I've checked this by printing what type of connection gets established in httpconnection.go.

Here's my client code in case it could help shed more light:

type Client struct {
    address string
    conn    signalr.Client
    updates   chan memory.Event
}

func NewClient(ctx context.Context, url string) (signalrinterface.Client, error) {
    events := make(chan memory.Event)
    rcvr := &receiver{
        updates: events,
    }
    c, err := signalr.NewClient(
        ctx,
        signalr.WithReceiver(rcvr),
        signalr.WithConnector(func() (signalr.Connection, error) {
            creationCtx, _ := context.WithTimeout(context.Background(), 2*time.Second)
            return signalr.NewHTTPConnection(creationCtx, url+"/events")
        }),
        signalr.TimeoutInterval(30*time.Second),
        signalr.KeepAliveInterval(10*time.Second),
        signalr.Logger(gokitlog.NewLogfmtLogger(io.Discard), false),
    )

    if err != nil {
        return nil, fmt.Errorf("failed to init SignalR client: %w", err)
    }

    return &Client{url, c, updates}, nil
}
philippseith commented 1 year ago

I think the problem is that keepAlive and timeout timers are initiated in the same select statement. KeepAlive is typically shorter, so the client pings and the timeout timer gets reseted. I tried to change that in https://github.com/philippseith/signalr/tree/bugfix/%23171_timeout_is_reset_with_ping @roks0n can you check if this branch solves the problem for you?

roks0n commented 1 year ago

Nice! Can confirm that timeout gets triggered now:

level=debug ts=2023-08-13T11:43:38.044085526Z protocol=JSON ts=2023-08-13T11:43:38.044086027Z caller=jsonhubprotocol.go:211 event=write message="{\"type\":6}\u001e"
level=debug ts=2023-08-13T11:43:48.048543041Z protocol=JSON ts=2023-08-13T11:43:48.048543692Z caller=jsonhubprotocol.go:211 event=write message="{\"type\":6}\u001e"
level=debug ts=2023-08-13T11:43:58.045968733Z protocol=JSON ts=2023-08-13T11:43:58.045969505Z caller=jsonhubprotocol.go:211 event=write message="{\"type\":7,\"error\":\"timeout interval elapsed (30s)\",\"allowReconnect\":false}\u001e"
level=debug ts=2023-08-13T11:43:58.04615836Z class=Client connection=vFWMNSp8GBI0tylTGCBXqQ hub=signalrclient.receiver ts=2023-08-13T11:43:58.046158821Z caller=loop.go:132 event="message loop ended"
level=info ts=2023-08-13T11:43:58.046197443Z connect="timeout interval elapsed (30s)"

I do get the context deadline exceeeded error now when the internet is back on again and the client tries to reconnect, I do understand if this error might not have much to do with this issue?

level=info ts=2023-08-13T11:44:42.043039085Z connect="Post \"https://example.com/events/negotiate\": context deadline exceeded"
philippseith commented 1 year ago

I guess it is the context that you pass to NewHttpConnection that is exceeding. I has to be recreated in the WithConnector option func for every reconnect.

roks0n commented 1 year ago

Ignore my last msg regarding context deadline exceeeded, the example code wasn't faulty just the internet connection took a bit longer to get back up because I wasnt connecting to internet directly. Feel free to mark this issue as closed.

Appreciate you having a look @philippseith! :beers: