Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/protocol: Revert unreleased changes related to closing connections #5688

Merged
merged 1 commit into from May 8, 2019

Conversation

imsodin
Copy link
Member

@imsodin imsodin commented May 3, 2019

As brought up by @calmh in the most recent PR (#5685) in a string of PRs related to handling closing of protocol connections, the way I introduced these changes wasn't particularly trust inspiring and the affected code is critical.

There were three involved changes, with follow-up fixes each:

  1. Propagating closing of connection in model to protocol: all: Send Close BEP msg on intentional disconnect #5440 -> lib/protocol: Fix potential deadlock when closing connection (ref #5440) #5442
  2. Sending cluster configs first: lib/protocol: Don't send any messages before cluster config #5646 -> lib/protocol: Don't close asyncMessage.done on success (fixes #5674) #5675
  3. Do not call the model after calling *model.Closed (fixes cluster config panic panic: bug: ClusterConfig called on closed or nonexistent connection  #4170): lib/model, lib/protocol: Wait for reader/writer loops on close (fixes #4170) #5657 -> lib/protocol: Fix yet another deadlock (fixes #5678) #5679 (unmerged: lib/protocol: Fix deadlock on sending close message  #5685)

My proposal for the next RC is to revert these changes (0229f04) and redo one unrelated change (337ae59). This brings us back to a state, that has been in place for a long time. There are known problems, but we were able to live with them for a long time, so we can again. I do intend to tackle this problems again for the next RC cycle. If you agree, I'll PR both commits separately, as I believe it makes for a cleaner history.

@imsodin imsodin force-pushed the protocolRewind branch 2 times, most recently from 72e8e1f to 0740519 Compare May 3, 2019 08:34
@calmh
Copy link
Member

calmh commented May 6, 2019

You're reverting quite far back now... The ones I would unwind are 9f358ec, ec7c88c, 19b51c9, 5da41f7, 04b9271 which is + 9f358ec and - 24ffd8b - c874118 compared to your PR, as those two commits are already included in releases.

@imsodin
Copy link
Member Author

imsodin commented May 6, 2019

Do you really want to revert 9f358ec too? That one has nothing to do with anything else, that's just code refactoring/hardening.

@calmh
Copy link
Member

calmh commented May 6, 2019

I was thinking it was on top of the other changes, but if it's entirely standalone then no, no need to revert that

@imsodin
Copy link
Member Author

imsodin commented May 6, 2019

Ok, one more question: With the recent extension of tests (#5682) fully reverting 5da41f7 doesn't pass model tests anymore. What about keeping the model.go part of that commit?

@calmh
Copy link
Member

calmh commented May 6, 2019

Use good judgement, no need to revert for revertings sake, just to back out the hole we were digging in protocol, to dig a better one later :)

@imsodin imsodin changed the title Staging: Protocol rewind lib/protocol: Revert unreleased changes related to closing connections May 6, 2019
@imsodin imsodin marked this pull request as ready for review May 6, 2019 19:08
@calmh calmh merged commit 283f39a into syncthing:master May 8, 2019
@calmh calmh added this to the v1.1.3 milestone May 8, 2019
@imsodin imsodin deleted the protocolRewind branch May 8, 2019 07:59
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label May 7, 2020
@syncthing syncthing locked and limited conversation to collaborators May 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants