all: Send Close BEP msg on intentional disconnect #5440
Conversation
This avoids waiting until next ping and timeout until the connection is actually closed both by notifying the peer of the disconnect and by immediately closing the local end of the connection after that. As a nice side effect, info level logging about dropped connections now have the actual reason in it, not a generic timeout error which looks like a real problem with the connection.
lib/protocol/protocol.go
Outdated
| }) | ||
| } | ||
|
|
||
| // close is called if there is an unexpected error during normal operation. | ||
| func (c *rawConnection) close(err error) { |
There was a problem hiding this comment.
I'm not super fond of the having both close() and Close(), the potential for confusion is enormous... I'm slightly confused even now. 😕 <- look, confused emoji
There was a problem hiding this comment.
Is internalClose suitable? It's a bit cryptic, but so is errorClose when Close takes an error as an argument as well and I couldn't think of anything better.
| c.sendClose <- asyncMessage{&Close{err.Error()}, done} | ||
| <-done | ||
|
|
||
| go c.commonClose(err) |
There was a problem hiding this comment.
Some sort of comment on why we go this? I have a vague feeling it's because this might be called from a protocol callback and there are potential deadlock issues?
There was a problem hiding this comment.
Why wouldn't we want an internal close here? Is it valid to close the connection multiple times? I assume that would panic on closing c.closed a second time. At which point the sync.once can move into the commonClose and commonClose can become internalClose?
There was a problem hiding this comment.
You lost me there. However you made me look at it again and I found a way this can deadlock and solving that indeed gets rid of commonClose (but introduces a second once). PR incoming
|
This could probably take care of #3379 somehow as well |
| default: | ||
| infoMsg = "Restarted" | ||
| errMsg = "restarting" | ||
| } |
There was a problem hiding this comment.
This fancy shortcutting switch behaviour takes a while to process, was pretty sure at one point this was an unfinished pr.
| @@ -128,6 +126,9 @@ var ( | |||
| errFolderNotRunning = errors.New("folder is not running") | |||
| errFolderMissing = errors.New("no such folder") | |||
| errNetworkNotAllowed = errors.New("network not allowed") | |||
| // errors about why a connection is closed | |||
| errIgnoredFolderRemoved = errors.New("folder no longer ignored") | |||
| errReplacingConnection = errors.New("replacing connection") | |||
There was a problem hiding this comment.
I thought (back in the day), the protocol had error codes to signify the reason in a cross implementation compatible way. Is that no longer the case?
There was a problem hiding this comment.
Nope, human readable string according to the docsé
Close
[...]
Fields
The reason field contains a human readable description of the error condition.
There was a problem hiding this comment.
Ah, it's response error code.
Perhaps we should add these errors at the protocol level, so all implementations use codes rather than strings?
| c.sendClose <- asyncMessage{&Close{err.Error()}, done} | ||
| <-done | ||
|
|
||
| go c.commonClose(err) |
There was a problem hiding this comment.
Why wouldn't we want an internal close here? Is it valid to close the connection multiple times? I assume that would panic on closing c.closed a second time. At which point the sync.once can move into the commonClose and commonClose can become internalClose?
…(ref syncthing#5440) (syncthing#5442)" This reverts commit c874118.
…(ref syncthing#5440) (syncthing#5442)" This reverts commit c874118.
st-review
added
the
frozen-due-to-age
Purpose
This avoids waiting until next ping and timeout until the connection is actually
closed both by notifying the peer of the disconnect and by immediately closing
the local end of the connection after that. As a nice side effect, info level
logging about dropped connections now have the actual reason in it, not a generic
timeout error which looks like a real problem with the connection.
Inspired by the logs containing tons of misleading lines about connection failures posted in https://forum.syncthing.net/t/syncthing-reconnect-disconnect-cycles-frequent/12675
Testing
Manual testing on test setup by un-/pausing folders.