RFC, derp/derphttp: Avoid redundand reconnects. #264
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. closeForReconnect() may be closing already reestablished connections. In the worst case, Send() and Recv(), running in concurrent loops, would be running connect() and closeForReconnect() calls in lockstep endlessly. 2. Reconnecting doesn't seem to make sense for non-networking errors (or at least not for all). e.g. derphttp.Client.Send() may fail with "packet too big", which is a local issue. Signed-off-by: Dmitry Adamushko <da@stablebits.net>
stablebits
force-pushed
the
close_for_reconnect
branch
from
c023bda
to
669199d
Compare
stablebits
commented
Apr 9, 2020
| @@ -234,13 +234,27 @@ func (c *Client) connect(ctx context.Context, caller string) (client *derp.Clien | |||
| return c.client, nil | |||
| } | |||
|
|
|||
| func reconnectRequired(err error) bool { | |||
There was a problem hiding this comment.
derp.Client methods could also return something like ErrUnusableTransport to indicate cases that require reconnecting (as opposed to cases when the current transport remains usable). The use of raw networking errors here is insufficient. Perhaps the error handling case should be addressed separately.
bradfitz
added a commit
that referenced
this pull request
Apr 11, 2020
Originally from @stablebits (Dmitry Adamushko) in: #264
|
@stablebits, I pushed 614eec1 which is the minimal version you'd proposed earlier. I'm not sure I want to get into the business of classifying errors at the moment. I think just saying any error means to give up on the client is enough for now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
connections. In the worst case, Send() and Recv(), running in
concurrent loops, would be running connect() and closeForReconnect()
calls in lockstep endlessly.
(or at least not for all). e.g. derphttp.Client.Send() may fail
with "packet too big", which is a local issue.
Signed-off-by: Dmitry Adamushko da@stablebits.net
P.S. Not tested locally.
This change is