Revert defer unlock in StayRTR AddVRPs

bgp / stayrtr

RPKI-To-Router server implementation in Go

BSD 3-Clause "New" or "Revised" License

91 stars 13 forks source link

Revert defer unlock in StayRTR AddVRPs #60

Closed BenCastricum closed 2 years ago

BenCastricum commented 2 years ago

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 commented 2 years ago

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

ties commented 2 years ago

And thanks for debugging this and your contribution!

lspgn commented 2 years ago

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)