Skip to content

fix a potential blocking issue when StreamHealth fails #3898

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.

Sign up for GitHub

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

Merged
sougou merged 2 commits into from
May 6, 2018
Merged

fix a potential blocking issue when StreamHealth fails #3898

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba486b3
fix a potential blocking issue when StreamHealth fails
demmer May 5, 2018
494c423
drop the lock in finalizeConn as well before calling Close()
demmer May 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions go/vt/discovery/healthcheck.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,17 +431,21 @@ func (hc *HealthCheckImpl) servingConnStats() map[string]int64 {
// checkConn().
func (hc *HealthCheckImpl) finalizeConn(hcc *healthCheckConn) {
hcc.mu.Lock()
if hcc.conn != nil {
hcc.conn.Close(hcc.ctx)
hcc.conn = nil
}
hccConn := hcc.conn
hccCtx := hcc.ctx
hcc.conn = nil
hcc.tabletStats.Up = false
hcc.tabletStats.Serving = false
// Note: checkConn() exits only when hcc.ctx.Done() is closed. Thus it's
// safe to simply get Err() value here and assign to LastError.
hcc.tabletStats.LastError = hcc.ctx.Err()
ts := hcc.tabletStats
hcc.mu.Unlock()

if hccConn != nil {
hccConn.Close(hccCtx)
}

if hc.listener != nil {
hc.listener.StatsUpdate(&ts)
}
Expand Down Expand Up @@ -511,7 +515,7 @@ func (hcc *healthCheckConn) stream(ctx context.Context, hc *HealthCheckImpl, cal

if err := conn.StreamHealth(ctx, callback); err != nil {
hcc.mu.Lock()
hcc.conn.Close(ctx)
log.Infof("StreamHealth failed from %v %v/%v (%v): %v", hcc.tabletStats.Tablet.GetAlias(), hcc.tabletStats.Tablet.GetKeyspace(), hcc.tabletStats.Tablet.GetShard(), hcc.tabletStats.Tablet.GetHostname(), err)
hcc.conn = nil
hcc.tabletStats.Serving = false
hcc.tabletStats.LastError = err
Expand All @@ -521,6 +525,7 @@ func (hcc *healthCheckConn) stream(ctx context.Context, hc *HealthCheckImpl, cal
if hc.listener != nil {
hc.listener.StatsUpdate(&ts)
}
conn.Close(ctx)
Copy link
Copy Markdown
Contributor

@sougou sougou May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be:
within lock:

hccConn := hcc.conn
hcc.conn = nil

and hccConn.Close(ctx) here.

Copy link
Copy Markdown
Member Author

@demmer demmer May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this as you suggested, but I can't think of any case in which conn != hcc.conn here and IMO this actually detracts from readability.

Copy link
Copy Markdown
Member Author

@demmer demmer May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record I removed this change again because we discussed in Slack and agreed that the original version was just as safe and was cleaner.

return
}
return
Expand Down Expand Up @@ -741,11 +746,12 @@ func (hc *HealthCheckImpl) WaitForInitialStatsUpdates() {
// GetConnection returns the TabletConn of the given tablet.
func (hc *HealthCheckImpl) GetConnection(key string) queryservice.QueryService {
hc.mu.RLock()
defer hc.mu.RUnlock()
hcc := hc.addrToConns[key]
if hcc == nil {
hc.mu.RUnlock()
return nil
}
hc.mu.RUnlock()
Copy link
Copy Markdown
Contributor

@sougou sougou May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix can cause problems because hcc.Conn can become nil after hc.mu.RUnlock, and Getconnection will end up returning nil.

Obtaining the read lock before releasing hc.mu will work. But then, we might as well leave it like before.

Copy link
Copy Markdown
Member Author

@demmer demmer May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed on Slack I don't actually think this changes the semantics of whether or not hcc.Conn is nil.

hcc.mu.RLock()
defer hcc.mu.RUnlock()
return hcc.conn
Expand Down