discovery: hold connMu when writing tabletHealthCheck health fields - #20319
discovery: hold connMu when writing tabletHealthCheck health fields#20319netliomax25-code wants to merge 6 commits into
Conversation
Signed-off-by: netliomax25-code <netliomax25@gmail.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
Bot
added
NeedsDescriptionUpdate
|
any update? |
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
TabletConnection still seems to read thc.Conn outside connMu. The PR moves Conn writes in closeConnection/finalizeConn under connMu, but HealthCheckImpl.TabletConnection still does the nil check with thc.Conn after releasing hc.mu and before taking connMu (go/vt/discovery/healthcheck.go:907). That leaves a remaining read/write race with the newly locked thc.Conn = nil writes in tablet_health_check.go, so the race fix is incomplete. It seems like we should also move the Conn nil check under connMu too, ideally via a small helper on tabletHealthCheck, and add a race regression that calls TabletConnection concurrently with stream close/finalize.
Thanks, @netliomax25-code !
|
@netliomax25-code We will also need a corresponding issue which lays out the problem we are fixing in this PR. Thanks! |
Signed-off-by: Kartik Kenchi <netliomax25@gmail.com>
|
Good catch, you're right that
Added Also opened #20419 to lay out the problem this PR fixes, and linked it in the description. |
mattlord
added
Type: Bug
Component: Query Serving
and removed
NeedsDescriptionUpdate
|
gentle ping |
|
An issue was created for this previously: #20325. I will close out mine in favor of yours, but for future reference, please look to see if there are any existing issues before creating new ones. Thanks! Additionally, your issue does not follow the bug report template. Please update it to do so as well. |
|
@netliomax25-code There are conflicts you'll need to resolve first. |
Signed-off-by: Kartik Kenchi <netliomax25@gmail.com> # Conflicts: # go/vt/discovery/healthcheck.go # go/vt/discovery/tablet_health_check.go
|
Sorry about the duplicate issue, I should have searched first. Both points addressed:
Re-ran |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
go/vt/discovery/tablet_health_check.go:120
setServingState's comment says logging happens from a separate goroutine to avoid holding the lock, but the code currently logs synchronously. With this PR making callers holdconnMuaroundsetServingState, theconnMumutex can now be held while logging, which can block concurrent health reads/updates if the logger output stalls. Consider capturing the values under the lock and logging asynchronously (or otherwise logging outside the critical section).
func (thc *tabletHealthCheck) setServingState(serving bool, reason string) {
if !thc.loggedServingState || (serving != thc.Serving) {
// Emit the log from a separate goroutine to avoid holding
// the th lock while logging is happening
thc.logger.Infof("HealthCheckUpdate(Serving State): tablet: %v serving %v => %v for %v/%v (%v) reason: %s",
topotools.TabletIdent(thc.Tablet),
thc.Serving,
serving,
thc.Tablet.GetKeyspace(),
thc.Tablet.GetShard(),
thc.Target.GetTabletType(),
reason,
)
|
Thanks for this fix! I verified the new tests fail under A few small things, none blocking:
|
…d test drains Signed-off-by: Kartik Kenchi <netliomax25@gmail.com>
|
All four addressed:
Re-ran the two regression tests and the full |
| // thc.connMu must be locked before calling this function. | ||
| func (thc *tabletHealthCheck) setServingState(serving bool, reason string) { |
There was a problem hiding this comment.
This seems like a valid point to me. No?
There was a problem hiding this comment.
Yes, it's a valid point, fixed in e6f0b52:
- Issue: with the log inside the connMu section, a CallbackLogger callback that re-enters the healthcheck (GetTabletHealthByAlias, TabletConnection) would try to take connMu again and deadlock the checkConn goroutine.
- Fix: setServingState now formats the message while holding connMu and returns a log function, and all four call sites invoke it right after unlocking. The state update and the log decision stay protected, only the logger call moved outside the lock.
- Added TestHealthCheckReentrantLoggerCallback, which installs a CallbackLogger that calls GetTabletHealthByAlias from inside the callback. On the previous revision it deadlocks and times out, with this change it passes, and the full ./go/vt/discovery/ suite is green under -race.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20319 +/- ##
===========================================
- Coverage 69.67% 69.64% -0.03%
===========================================
Files 1614 12 -1602
Lines 216793 1835 -214958
===========================================
- Hits 151044 1278 -149766
+ Misses 65749 557 -65192
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A CallbackLogger callback that re-enters the healthcheck (e.g. via GetTabletHealthByAlias) would deadlock on connMu if setServingState invoked the logger while holding it. setServingState now formats the message under the lock and returns a log function that callers invoke after unlocking. Signed-off-by: Kartik Kenchi <netliomax25@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
go/vt/discovery/healthcheck_test.go:333
- This goroutine spins in a tight loop due to the
defaultcase, which can peg a CPU core and make tests noisier/flakier under load. Consider adding a small backoff (e.g.runtime.Gosched()or a shorttime.Sleep) or driving the reads with a ticker/channel instead of a busy loop.
for {
select {
case <-stop:
return
default:
_, _ = hc.GetTabletHealthByAlias(tablet.Alias)
}
}
go/vt/discovery/healthcheck_test.go:398
- This is also a tight busy loop via
default, which can unnecessarily consume CPU during the 200ms test window. Add a small yield/backoff or use a ticker to reduce contention while still exercising the race scenario.
for {
select {
case <-stop:
return
default:
_, _ = hc.TabletConnection(ctx, tablet.Alias, nil)
}
}
| if logServingChange != nil { | ||
| logServingChange() | ||
| } | ||
| _ = conn.Close(ctx) |
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
I don’t see any remaining correctness or performance issues at the latest HEAD. The open nil-connection concern does not seem reachable: closeConnection is only called after Connection returned a non-nil connection and that connection’s StreamHealth failed. finalizeConn runs later on the same goroutine, and no other path clears Conn.
One small non-blocking nit: the connMu comment still says it protects only Conn, but it now protects all mutable health fields. I think we should update that comment to document the synchronization contract. Otherwise, this looks good to me.
| return thc.Connection(ctx), nil | ||
| conn := thc.currentConnection() | ||
| if conn == nil { | ||
| return nil, vterrors.Errorf(vtrpc.Code_NOT_FOUND, "tablet: %v is either down or nonexistent", alias) |
There was a problem hiding this comment.
IMO we should not have identical errors for different cases. Can we at least add something in parens to indicate that there was no connection?
There was a problem hiding this comment.
Done in b26019a, the no-connection case now says "is either down or nonexistent (no health check connection)" so the two failures are distinguishable. The e2e assertion in vtgate_test.go matches on the shared substring, so it still passes.
Signed-off-by: Kartik Kenchi <netliomax25@gmail.com>
|
Both points from the review addressed in b26019a:
Agreed on the nil |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
go/vt/discovery/tablet_health_check.go:171
connectionLockedperforms dialing (tabletconn.GetDialer()(...)) in a function that is intended to be called whileconnMuis held (per naming and current call patterns). That directly conflicts with the newconnMucontract comment stating it must not be held across network IO/dialing, and can cause head-of-line blocking (and potentially deadlocks if dialing/logging paths re-enter). Refactor to releaseconnMubefore dialing, then re-lock to publishthc.Conn(double-checking whether another goroutine already set it), and close any redundant connection outside the lock.
func (thc *tabletHealthCheck) connectionLocked(ctx context.Context) queryservice.QueryService {
if thc.Conn == nil {
conn, err := tabletconn.GetDialer()(ctx, thc.Tablet, grpcclient.FailFast(true))
go/vt/discovery/healthcheck.go:925
- This changes
TabletConnectionsemantics from 'return (and potentially establish) a connection' (previouslythc.Connection(ctx)) to 'only return an already-established connection'. That can break callers by returning NOT_FOUND during transient periods (e.g., right afterAddTablet, during redial, or before the first stream is up). If the API is expected to provide a usable connection when possible, consider keeping the dialing behavior and fixing the underlying synchronization (e.g., by making dialing happen outsideconnMu, per theconnMucontract). If the new behavior is desired, consider updating the error code/message to distinguish 'temporarily unavailable' from 'nonexistent'.
func (hc *HealthCheckImpl) TabletConnection(ctx context.Context, alias *topodata.TabletAlias, target *query.Target) (queryservice.QueryService, error) {
thc := hc.registeredHealthCheck(alias)
if thc == nil {
return nil, vterrors.Errorf(vtrpc.Code_NOT_FOUND, "tablet: %v is either down or nonexistent", alias)
}
conn := thc.currentConnection()
if conn == nil {
return nil, vterrors.Errorf(vtrpc.Code_NOT_FOUND, "tablet: %v is either down or nonexistent (no health check connection)", alias)
}
return conn, nil
go/vt/discovery/healthcheck_test.go:337
- These test goroutines busy-spin on the
defaultcase, potentially pegging a CPU core during the sleep window and making CI noisier/slower. Consider adding a small backoff (e.g.,runtime.Gosched()or a shorttime.Sleep) or using a ticker/channel-driven loop to reduce CPU usage (same pattern also appears in theTabletConnectionreader loop).
go func() {
defer wg.Done()
for {
select {
case <-stop:
return
default:
_, _ = hc.GetTabletHealthByAlias(tablet.Alias)
}
}
}()
Description
tabletHealthCheck.SimpleCopy(andconnectionLocked) readTarget,Serving,Stats,LastErrorandPrimaryTermStartTimeunderconnMu, butprocessResponse, the timeout branch ofcheckConn,closeConnectionandfinalizeConnwrite those same fields without holdingconnMu.HealthCheck.TabletConnectionalso readthc.ConnoutsideconnMu.SimpleCopyis reachable from another goroutine throughHealthCheck.GetTabletHealthByAlias, andthc.ConnthroughHealthCheck.TabletConnection, while the per-tabletcheckConngoroutine is applying a streaming health update or closing the connection, sogo test -racereports read/write races on those fields.connMuaround the field writes at all four sites, releasing it beforeSimpleCopyand before any connectionCloseso the lock is never held across network IO, and added acurrentConnectionhelper soTabletConnectionreadsthc.ConnunderconnMu.The added regression tests drive
processResponse/GetTabletHealthByAliasandTabletConnection/stream-close concurrently and fail under-racewithout the change.Related Issue(s)
Checklist
Deployment Notes
None. Internal locking fix with no user-visible behavior change.