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 deadlock in messager and health streamer #17230

Closed GuptaManan100 closed 1 week ago

GuptaManan100 commented 1 week ago

Description

This PR fixes the deadlocks in messager and health streamer as pointed out in #17229.

The fix to the deadlock for both the cases was to use two different mutexes. One that only protects against calling setup functions like Open and Close concurrently, and the other one that actually protects the fields. This helps in removing the deadlock since the function that runs on a broadcast from the schema engine would acquire a different mutex than the call to Close.

Related Issue(s)

Checklist

Deployment Notes

vitess-bot[bot] commented 1 week 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 week ago

Codecov Report

Attention: Patch coverage is 89.18919% with 4 lines in your changes missing coverage. Please review.

Project coverage is 67.39%. Comparing base (c72bbdc) to head (3901fc2). Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
go/vt/vttablet/tabletserver/health_streamer.go 82.60% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #17230 +/- ## ======================================= Coverage 67.39% 67.39% ======================================= Files 1570 1570 Lines 252903 252912 +9 ======================================= + Hits 170438 170461 +23 + Misses 82465 82451 -14 ```

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

derekperkins commented 1 week ago

For some historical context, when the original code was written, defer still had a somewhat significant penalty, hence the lack of defer mu.Unlock. That was resolved in upstream Go in v1.14.