Tibber: resubscribe on clean disconnect #18643
Conversation
|
@andig I refactored the existing subscription logic into a function (subscribe), and added an optional unsubscribe to it (in case we have a subscriptionId from a previous subscribe). For the reconnect loop, I am using the presence of an error to decide wether to just restart Run() or also additionally re-subscribe. Maybe we can also get rid of that decision by simply always resubscribing? WDYT? |
| @@ -133,18 +122,56 @@ func NewTibberFromConfig(ctx context.Context, other map[string]interface{}) (api | |||
| case <-ctx.Done(): | |||
| return | |||
| } | |||
|
|
|||
| if err != nil { | |||
There was a problem hiding this comment.
can we do this above in the first if?
| } | ||
| }() | ||
|
|
||
| return t, nil | ||
| } | ||
|
|
||
| func subscribe(t *Tibber, client *graphql.SubscriptionClient, cc struct { |
There was a problem hiding this comment.
we should use the actual parameters instead of anonymous struct type.
There was a problem hiding this comment.
und kann das nicht eine Methode vom Pulse sein?
| }) error { | ||
| if t.subscriptionId != "" { | ||
| if err := client.Unsubscribe(t.subscriptionId); err != nil { | ||
| return err |
There was a problem hiding this comment.
Log statt return? Die mag ja schon stale sein?
There was a problem hiding this comment.
Ich wäre sogar dafür, das im ersten Schritte einfach weg zu lassen. In allen Fehlern die wir kennen hat uns ja der Server schon die Subscription entzogen.
| } | ||
| case <-time.After(cc.Timeout): | ||
| return nil, api.ErrTimeout | ||
| if err := subscribe(t, client, cc); err != nil { |
There was a problem hiding this comment.
Den Begriff "subscribe" haben wir jetzt 2x
|
Ich habs nicht versucht im Detail zu verstehen, aber die Idee ist klar- bin gespannt... |
|
ping @GrimmiMeloni |
|
Sorry. Gerade unpässlich 🤒 |
|
Ohje- gute Besserung! |
fix #17925
Resubscribes in case of a clean exit.