make InitialReconnectBackoffWindow a function and add ForceInitialBackoff #129
Conversation
rpc/connection.go
Outdated
| @@ -336,7 +336,12 @@ type ConnectionOpts struct { | |||
| // InitialReconnectBackoffWindow, if non zero, causes a random backoff | |||
| // before reconnecting. The random backoff timer is fast-forward-able by | |||
| // passing in a WithFireNow(ctx) into a RPC call. | |||
| InitialReconnectBackoffWindow time.Duration | |||
| InitialReconnectBackoffWindow func() time.Duration | |||
| // As the name suggests, we normally skip the "initial REconnect backoff" | |||
There was a problem hiding this comment.
I have a slight preference for an interface here which includes a GetReconnectBackoffWindow() method. But I'm OK with this too.
Please update the comment to reflect that this field is now a function.
The other thing is that I actually don't remember why this was named InitialReconnectBackOffWindow rather than ReconnectBackOffWindow. Maybe I planned to have it changeable, but that doesn't seem to be the case anymore, so the naming is a bit confusing now. So if you think the latter is better, feel free to change it!
There was a problem hiding this comment.
The reason it's not ReconnectBackOffWindow is because there's also a ReconnectBackoff function immediately above. Perhaps the Right Thing would be to combine the two, but I tried not to change more than I needed to here.
|
cc @songgao |
rpc/connection.go
Outdated
| InitialReconnectBackoffWindow func() time.Duration | ||
| // As the name suggests, we normally skip the "initial REconnect backoff" | ||
| // the very first time we try to connect. However, some callers instantiate | ||
| // new Connection objects after a disconnect, and they need the "first |
There was a problem hiding this comment.
This is probably not mine to review, but IMO all reconnects should happen in this package, as opposed to making new rpc.Connection objects :-) Otherwise we might have redundant rpc.Connection lying around, and the redundant one might be trying to reconnect itself as well and continue with RPCs. We’ve had pretty stable reconnects for KBFS servers after moving the reconnect logic here.
There was a problem hiding this comment.
You'll have to talk to @mmaxim about the problems he ran into with reconnect.
I kind of get the impression that someone's going to want to rewrite this entire class, and then salt the earth so that the original never grows again :)
There was a problem hiding this comment.
I kind of get the impression that someone's going to want to rewrite this entire class
Yeah I’ve had the impulse to do so a few times, but was afraid I was gonna introduce bugs or it would meet resistance so I ended up never trying 😅
rpc/connection.go
Outdated
| @@ -336,7 +336,12 @@ type ConnectionOpts struct { | |||
| // InitialReconnectBackoffWindow, if non zero, causes a random backoff | |||
| // before reconnecting. The random backoff timer is fast-forward-able by | |||
| // passing in a WithFireNow(ctx) into a RPC call. | |||
| InitialReconnectBackoffWindow time.Duration | |||
| InitialReconnectBackoffWindow func() time.Duration | |||
| // As the name suggests, we normally skip the "initial REconnect backoff" | |||
There was a problem hiding this comment.
I have a slight preference for an interface here which includes a GetReconnectBackoffWindow() method. But I'm OK with this too.
Please update the comment to reflect that this field is now a function.
The other thing is that I actually don't remember why this was named InitialReconnectBackOffWindow rather than ReconnectBackOffWindow. Maybe I planned to have it changeable, but that doesn't seem to be the case anymore, so the naming is a bit confusing now. So if you think the latter is better, feel free to change it!
oconnor663
force-pushed
the
jack/CORE-6527/backoff
branch
from
1a4a7d1
to
34c8b94
Compare
oconnor663
force-pushed
the
jack/CORE-6527/backoff
branch
from
34c8b94
to
df335eb
Compare
These changes will allow client to back off of gregor reconnects, based on whether the current device is active in chat.
r? @mmaxim