vitessio / vitess

Vitess is a database clustering system for horizontal scaling of MySQL.
http://vitess.io
Apache License 2.0
18.7k stars 2.1k forks source link

Fix potential deadlock in health streamer #17261

Closed GuptaManan100 closed 23 hours ago

GuptaManan100 commented 1 day ago

Description

While we fixed a couple of deadlocks in https://github.com/vitessio/vitess/pull/17230, one was left pending. It was found while testing for deadlocks in https://github.com/vitessio/vitess/pull/17238.

The deadlock is similar to the ones fixed in https://github.com/vitessio/vitess/pull/17230, it was occurring between MakePrimary and the reload. The fix employed is to let go of the lock once it is not required instead of defering its unlocking.

Related Issue(s)

Checklist

Deployment Notes

vitess-bot[bot] commented 1 day ago

Review Checklist

Hello reviewers! :wave: Please follow this checklist when reviewing this Pull Request.

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov[bot] commented 1 day ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 67.39%. Comparing base (2c6e053) to head (eadb128). Report is 5 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #17261 +/- ## ======================================= Coverage 67.39% 67.39% ======================================= Files 1570 1573 +3 Lines 252970 253060 +90 ======================================= + Hits 170484 170559 +75 - Misses 82486 82501 +15 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features:

mdlayher commented 1 day ago

If we're able, you could consider swapping this for the fairly new atomic.Bool in Go to eliminate this lock all together!

arthurschreiber commented 1 day ago

If we're able, you could consider swapping this for the fairly new atomic.Bool in Go to eliminate this lock all together!

I don't think that's possible here, because we depend on a consistent view over multiple fields (isServingPrimary, state).

mdlayher commented 1 day ago

If a consistent view over multiple fields is necessary then https://pkg.go.dev/sync/atomic#Pointer with a struct is also an option. No strong feelings, just wanted to offer a suggestion to eliminate the lock.