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

Revert defer unlock in StayRTR AddVRPs #60

Merged
merged 1 commit into from
Jan 26, 2022
Merged

Conversation

BenCastricum
Copy link

vrplock needs to be unlocked before AddVRPsDiff() because AddVRPsDiff needs a full lock.

I added some debug logging found this deadlock

INFO[0000] new cache file: Updating sha256 hash -> da753c7804d6f386bf303fed6931853eaaca0771ba160ef7fdbebb17e899d78b
INFO[0001] New update (306189 uniques, 306189 total prefixes).
INFO[0001] RLocking vrplock in AddVRPs
INFO[0002] RLocking vrplock in AddVRPsDiff
INFO[0002] RUnlocked vrplock in AddVRPsDiff
INFO[0002] Locking vrplock in AddVRPsDiff
...

vrplock needs to be unlocked before AddVRPsDiff() because AddVRPsDiff needs a full lock.

I added some debug logging found this deadlock

INFO[0000] new cache file: Updating sha256 hash  -> da753c7804d6f386bf303fed6931853eaaca0771ba160ef7fdbebb17e899d78b
INFO[0001] New update (306189 uniques, 306189 total prefixes).
INFO[0001] RLocking vrplock in AddVRPs
INFO[0002] RLocking vrplock in AddVRPsDiff
INFO[0002] RUnlocked vrplock in AddVRPsDiff
INFO[0002] Locking vrplock in AddVRPsDiff
...
@ties
Copy link
Collaborator

ties commented Jan 26, 2022

This reverts 3726782 in AddVRPs. Looks good to me. Will ask @lspgn for a second opinion.

@ties ties requested review from lspgn and ties January 26, 2022 14:08
@ties
Copy link
Collaborator

ties commented Jan 26, 2022

And thanks for debugging this and your contribution!

@job job merged commit c4ac625 into bgp:master Jan 26, 2022
@lspgn
Copy link
Contributor

lspgn commented Jan 28, 2022

Good catch.
One pattern I've been seeing which could improve this code


func (s *Server) addVRPsDiff(diff []VRP) {
   [...]
}


func (s *Server) AddVRPsDiff(diff []VRP) {
	s.vrplock.RLock()
	s.addVRPsDiff(diff)
	s.vrplock.RUnlock()
}

And the internal functions would call s.addVRPsDiff(diff)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants