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/connections: More fine grained locking (fixes #3066) #3069

Closed
wants to merge 4 commits into from
Closed

lib/connections: More fine grained locking (fixes #3066) #3069

wants to merge 4 commits into from

Conversation

calmh
Copy link
Member

@calmh calmh commented May 9, 2016

Purpose

This fixes the deadlock by reducing where we hold the various locks. To
start with it splits up the existing "mut" into a "listenersMut" and a
"curConMut" as these are the two things being protected and I can see no
relation between them that requires a shared lock. It also moves all
model calls outside of the lock, as I see no reason to hold the lock
while calling the model (and it's risky, as proven).

Testing

Things connect, now.

This fixes the deadlock by reducing where we hold the various locks. To
start with it splits up the existing "mut" into a "listenersMut" and a
"curConMut" as these are the two things being protected and I can see no
relation between them that requires a shared lock. It also moves all
model calls outside of the lock, as I see no reason to hold the lock
while calling the model (and it's risky, as proven).
@AudriusButkevicius
Copy link
Member

So this potentially brings back the connected to already connected issue, if we don't lock around AddConnection and ConnectedTo

@calmh
Copy link
Member Author

calmh commented May 9, 2016

How does that happen, again?

@AudriusButkevicius
Copy link
Member

AudriusButkevicius commented May 9, 2016

T1 in redial checks connectionType map, no value
T2 does model.AddConnection in handle
T2 sets connectionType map
T1 checks model.ConnectedTo and prints message

@calmh
Copy link
Member Author

calmh commented May 9, 2016

So if one connection comes in at the same time as we're dialing, one will get hit by "already connected" which is fine? We could reduce the window a bit though as connect() grabs ct quite early and the looks at it much later. There is also some optimizations to be done I think, as we could skip devices that are already connected with the maximum dialer preference and so. We keep doing discovery for devices as well, even though we are already connected...

@AudriusButkevicius
Copy link
Member

I am as always on my phone so hard to have a proper discussion, though what you are saying sounds like true.

I think the caching discovery mux absorbs the frequent lookups?

@calmh
Copy link
Member Author

calmh commented May 9, 2016

Some of them, but I think it still lets through a global discovery query per five or ten minutes or something. But that's an easy optimization for another commit. :)

@AudriusButkevicius
Copy link
Member

So whats the plan? Are we pushing this and following it up or whats the plan.

@AudriusButkevicius
Copy link
Member

@st-review merge

@calmh
Copy link
Member Author

calmh commented May 9, 2016

From my POV yes as it fixes the problem I see, and I don't think I see the other problem if it's here. :)

@st-review
Copy link

👌 Merged as 31f6418. Thanks, @calmh!

st-review pushed a commit that referenced this pull request May 9, 2016
This fixes the deadlock by reducing where we hold the various locks. To
start with it splits up the existing "mut" into a "listenersMut" and a
"curConMut" as these are the two things being protected and I can see no
relation between them that requires a shared lock. It also moves all
model calls outside of the lock, as I see no reason to hold the lock
while calling the model (and it's risky, as proven).

GitHub-Pull-Request: #3069
@st-review st-review closed this May 9, 2016
@AudriusButkevicius
Copy link
Member

Well the other problem is definately there, as thats what I tried to fix by using wider scope for the locks.

@calmh
Copy link
Member Author

calmh commented May 9, 2016

I remember that issue being about us not switching connections properly. I'll see if I can find it later.

@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 Jun 16, 2017
@syncthing syncthing locked and limited conversation to collaborators Jun 16, 2017
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