philippseith / signalr

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

0.6.1: connect=*signalr.webSocketConnection: context canceled #164

Closed andig closed 1 year ago

andig commented 1 year ago

Since 0.6.1 I'm no longer able to connect:

[easee ] TRACE 2023/04/05 16:10:05 POST https://api.easee.cloud/hubs/chargers/negotiate
[easee ] TRACE 2023/04/05 16:10:05 {"negotiateVersion":0,"connectionId":"NhkJRDibpdJNrtcaf78kQQ","availableTransports":[{"transport":"WebSockets","transferFormats":["Text","Binary"]},{"transport":"ServerSentEvents","transferFormats":["Text"]},{"transport":"LongPolling","transferFormats":["Text","Binary"]}]}
[easee ] TRACE 2023/04/05 16:10:05 level=info connection=NhkJRDibpdJNrtcaf78kQQ event=handshake sent msg={"protocol":"json","version":1} error=*signalr.webSocketConnection: context canceled
[easee ] TRACE 2023/04/05 16:10:05 level=info connect=*signalr.webSocketConnection: context canceled
[easee ] TRACE 2023/04/05 16:10:06 level=info connection=NhkJRDibpdJNrtcaf78kQQ event=handshake sent msg={"protocol":"json","version":1} error=*signalr.webSocketConnection: context canceled
[easee ] TRACE 2023/04/05 16:10:06 level=info connect=*signalr.webSocketConnection: context canceled
[easee ] TRACE 2023/04/05 16:10:07 level=info connection=NhkJRDibpdJNrtcaf78kQQ event=handshake sent msg={"protocol":"json","version":1} error=*signalr.webSocketConnection: context canceled
[easee ] TRACE 2023/04/05 16:10:07 level=info connect=*signalr.webSocketConnection: context canceled
[easee ] TRACE 2023/04/05 16:10:08 level=info connection=NhkJRDibpdJNrtcaf78kQQ event=handshake sent msg={"protocol":"json","version":1} error=*signalr.webSocketConnection: context canceled
[easee ] TRACE 2023/04/05 16:10:08 level=info connect=*signalr.webSocketConnection: context canceled
[easee ] TRACE 2023/04/05 16:10:09 level=info connection=NhkJRDibpdJNrtcaf78kQQ event=handshake sent msg={"protocol":"json","version":1} error=*signalr.webSocketConnection: context canceled
[easee ] TRACE 2023/04/05 16:10:09 level=info connect=*signalr.webSocketConnection: context canceled
[easee ] TRACE 2023/04/05 16:10:13 level=info connection=NhkJRDibpdJNrtcaf78kQQ event=handshake sent msg={"protocol":"json","version":1} error=*signalr.webSocketConnection: context canceled
[easee ] TRACE 2023/04/05 16:10:13 level=info connect=*signalr.webSocketConnection: context canceled
[easee ] TRACE 2023/04/05 16:10:17 level=info connection=NhkJRDibpdJNrtcaf78kQQ event=handshake sent msg={"protocol":"json","version":1} error=*signalr.webSocketConnection: context canceled
[easee ] TRACE 2023/04/05 16:10:17 level=info connect=*signalr.webSocketConnection: context canceled

Last known good is bd5ffb679356

philippseith commented 1 year ago

We need a regression test for your use case. Can you supply one? (That would also help me to fix the issue)

andig commented 1 year ago

We‘ll try to figure out the commit first and then see what makes our case special. Keep you posted!

PS.: I‘m suspicious of https://github.com/philippseith/signalr/pull/152 since it changes the context of the socket connection.

andig commented 1 year ago

Confirmed. Last known good is 7237db3.

andig commented 1 year ago

According to https://github.com/philippseith/signalr/pull/152/files#diff-f2a77965ce3957d1223d145c0f1fc2ff731ca1b18e69ba85bbcec753ed10e676L42-L45:

// NewHTTPConnection creates a signalR HTTP Connection for usage with a Client.
// ctx can be used to cancel the SignalR negotiation during the creation of the Connection
// but not the Connection itself.

Due to https://github.com/philippseith/signalr/pull/152/files#diff-f2a77965ce3957d1223d145c0f1fc2ff731ca1b18e69ba85bbcec753ed10e676L133-L134 this is no longer true since the context now also controls the connection. This breaks our init:

ctx, cancel := context.WithTimeout(context.Background(), request.Timeout)
defer cancel()

return signalr.NewHTTPConnection(ctx, "https://api.easee.cloud/hubs/chargers",
    signalr.WithHTTPClient(c.Client),
    signalr.WithHTTPHeaders(func() (res http.Header) {
        return http.Header{
            "Authorization": []string{fmt.Sprintf("Bearer %s", tok.AccessToken)},
        }
    }),
)

152 is imho wrong as it breaks an api promise.

philippseith commented 1 year ago

If I only had sticked to the change which really solved #151 :-/ - done to much. I will change this again. Do you see the need to give the possibility to cancel the connection itself?

andig commented 1 year ago

Thank you @philippseith. Maybe also issue point release?