Skip to content

Conversation

qingyang-hu
Copy link
Collaborator

GODRIVER-3107

Summary

Fix the leaky rttMonitor.runHellos() routine when a disconnect occurs before a connection takes place.

Background & Motivation

There is a chance that rttMonitor.disconnect() is called before rttMonitor.connect() as described in PR #1376. In that case, the condition in disconnect() triggers a shortcut return which makes the runHellos() routine impossible to be closed.

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot added the review-priority-low Low Priority PR for Review: within 3 business days label Mar 29, 2024
Copy link
Contributor

API Change Report

No changes found!

s.pool.close(ctx)

s.closewg.Wait()
s.rttMonitor.disconnect()
Copy link
Collaborator Author

@qingyang-hu qingyang-hu Mar 29, 2024

Choose a reason for hiding this comment

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

It does not cause any actual impact whether or not this line is moved. I put it here as a "fail-safe" that disconnects after the update() exits, so rttMonitor.connect() is more likely to be called beforehand.

@qingyang-hu qingyang-hu marked this pull request as ready for review March 29, 2024 20:46
@qingyang-hu qingyang-hu added review-priority-normal Medium Priority PR for Review: within 1 business day and removed review-priority-low Low Priority PR for Review: within 3 business days labels Apr 1, 2024

processErrorLock sync.Mutex
rttMonitor *rttMonitor
monitorOnce *sync.Once
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since Server is always passed as a pointer, this sync.Once can be a non-pointer type (like sync.Mutex above). If it is ever copied, the go vet linter will cause an error.

Copy link
Collaborator

@matthewdale matthewdale left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-priority-normal Medium Priority PR for Review: within 1 business day
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants