-
Notifications
You must be signed in to change notification settings - Fork 901
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.
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
GODRIVER-3156 Detect and discard closed idle connections. #1815
GODRIVER-3156 Detect and discard closed idle connections. #1815
Conversation
// If the connection has been idle for less than 10 seconds, skip the liveness | ||
// check. | ||
idleStart, ok := c.idleStart.Load().(time.Time) | ||
if !ok || idleStart.Add(10*time.Second).After(time.Now()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the significance of 10 seconds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reference was the PyMongo driver, which checks connection liveness when it's idle for >1 second. I wanted to mitigate the risk that checking liveness too often would cause performance problems (since a liveness check takes at least 1ms), so I increased that threshold to 10s.
This looks relevant: |
API Change ReportNo changes found! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I recommend adding a unit test for isAlive:
func TestConnection_IsAlive(t *testing.T) {
t.Run("idle time lt 10 seconds", func(t *testing.T) {
mockConn := &drivertest.ChannelNetConn{}
conn := &connection{
nc: mockConn,
}
conn.idleStart.Store(time.Now())
assert.True(t, conn.isAlive())
})
t.Run("idle time gt 10 seconds", func(t *testing.T) {
mockConn := &drivertest.ChannelNetConn{
ReadResp: make(chan []byte, 1),
}
mockConn.ReadResp <- []byte{5, 0, 0, 0, 0}
conn := &connection{
nc: mockConn,
}
conn.idleStart.Store(time.Now().Add(-11 * time.Second))
assert.False(t, conn.isAlive())
})
t.Run("read error", func(t *testing.T) {
mockConn := &drivertest.ChannelNetConn{
ReadErr: make(chan error, 1),
}
mockConn.ReadErr <- errors.New("")
conn := &connection{
nc: mockConn,
}
conn.idleStart.Store(time.Now().Add(-11 * time.Second))
assert.False(t, conn.isAlive())
})
// TODO:
// case: deadline exceeded
}
drivers-pr-bot please backport to release/1.17 |
Co-authored-by: Steven Silvester <[email protected]> (cherry picked from commit 6952473)
…lease/1.17] (#1841) Co-authored-by: Matt Dale <[email protected]>
Co-authored-by: Steven Silvester <[email protected]>
…ster] (#1842) Co-authored-by: Steven Silvester <[email protected]>
* release/1.17: Use different credentials for merge-up PRs (#1968) Add GitHub Actions workflow for merge ups (#1962) BUMP v1.17.3 GODRIVER-3448 Limit GOMAXPROCS for fuzz tests (#1939) [v1] (#1943) [release/1.17] (#1957) Update reviewers.txt (#1855) [v1] (#1883) [release/1.17] (#1958) Cherry pick 1.17.2 work to release/1.17 (#1956) GODRIVER-3470 Correct BSON unmarshaling logic for null values (#1924) (#1955) GODRIVER-3370 Add bypassEmptyTsReplacement option. (#1927) [release/1.17] (#1954) GODRIVER-3340 Bump github.com/klauspost/compress from 1.13.6 to 1.16.7 [release/1.17] (#1880) GODRIVER-3374 Add ReadCompressedCompressedMessage back to wiremessage API (#1870) Fix data race in 'discard connections' pool test. [v1] (#1877) Bump golangci-lint for 1.23 compatibility [v1] (#1875) BUMP v1.17.1 GODRIVER-3156 Detect and discard closed idle connections. (#1815) [release/1.17] (#1841) GODRIVER-3313 [release/1.17] Skip CSOT spec tests on Windows and macOS. (#1838) GODRIVER-3358 [release/1.17] Do not override authSource from TXT record (#1840) GODRIVER-2589 [release/1.17] Clarify `*Cursor.All()` behavior in comment. (#1839) DEVPROD-10453 Use assume_role for s3 uploads [release/1.17] (#1824) (#1837) update repo metadata
* commit '9c485751': BUMP v1.17.3 GODRIVER-3448 Limit GOMAXPROCS for fuzz tests (#1939) [v1] (#1943) [release/1.17] (#1957) Update reviewers.txt (#1855) [v1] (#1883) [release/1.17] (#1958) Cherry pick 1.17.2 work to release/1.17 (#1956) GODRIVER-3470 Correct BSON unmarshaling logic for null values (#1924) (#1955) GODRIVER-3370 Add bypassEmptyTsReplacement option. (#1927) [release/1.17] (#1954) GODRIVER-3340 Bump github.com/klauspost/compress from 1.13.6 to 1.16.7 [release/1.17] (#1880) GODRIVER-3374 Add ReadCompressedCompressedMessage back to wiremessage API (#1870) Fix data race in 'discard connections' pool test. [v1] (#1877) Bump golangci-lint for 1.23 compatibility [v1] (#1875) BUMP v1.17.1 GODRIVER-3156 Detect and discard closed idle connections. (#1815) [release/1.17] (#1841) GODRIVER-3313 [release/1.17] Skip CSOT spec tests on Windows and macOS. (#1838) GODRIVER-3358 [release/1.17] Do not override authSource from TXT record (#1840) GODRIVER-2589 [release/1.17] Clarify `*Cursor.All()` behavior in comment. (#1839) DEVPROD-10453 Use assume_role for s3 uploads [release/1.17] (#1824) (#1837) update repo metadata
* commit '0dc2e05e': (184 commits) GODRIVER-3448 Limit GOMAXPROCS for fuzz tests (#1939) [v1] (#1943) GODRIVER-3470 Correct BSON unmarshaling logic for null values (#1924) GODRIVER-3370 Add bypassEmptyTsReplacement option. (#1927) BUMP v1.17.2 GODRIVER-3436 Avoid initializing null data given custom decoder (#1902) GODRIVER-3340 Add a test for goroutine leaks. (#1874) Update reviewers.txt (#1855) [v1] (#1883) Fix data race in 'discard connections' pool test. [v1] (#1877) Bump golangci-lint for 1.23 compatibility [v1] (#1875) GODRIVER-3340 Bump github.com/klauspost/compress from 1.13.6 to 1.16.7 [v1] (#1869) GODRIVER-3374 Add ReadCompressedCompressedMessage back to wiremessage API (#1870) GODRIVER-3156 Detect and discard closed idle connections. (#1815) GODRIVER-3358 Do not override authSource from TXT record (#1830) DEVPROD-10453 Use assume_role for s3 uploads [v1] (#1824) GODRIVER-2589 Clarify `*Cursor.All()` behavior in comment. (#1804) GODRIVER-3313 Skip CSOT spec tests on Windows and macOS. (#1818) BUMP v1.17.0 GODRIVER-3302 Handle malformatted message length properly. (#1758) GODRIVER-3312 Use remaining test secrets from the vault [v1] (#1811) Remove GCP from supplied callback example (#1809) ...
* v1: Use different credentials for merge-up PRs (#1968) Add GitHub Actions workflow for merge ups (#1962) BUMP v1.17.3 GODRIVER-3448 Limit GOMAXPROCS for fuzz tests (#1939) [v1] (#1943) [release/1.17] (#1957) Update reviewers.txt (#1855) [v1] (#1883) [release/1.17] (#1958) Cherry pick 1.17.2 work to release/1.17 (#1956) GODRIVER-3470 Correct BSON unmarshaling logic for null values (#1924) (#1955) GODRIVER-3370 Add bypassEmptyTsReplacement option. (#1927) [release/1.17] (#1954) GODRIVER-3340 Bump github.com/klauspost/compress from 1.13.6 to 1.16.7 [release/1.17] (#1880) GODRIVER-3374 Add ReadCompressedCompressedMessage back to wiremessage API (#1870) Fix data race in 'discard connections' pool test. [v1] (#1877) Bump golangci-lint for 1.23 compatibility [v1] (#1875) BUMP v1.17.1 GODRIVER-3156 Detect and discard closed idle connections. (#1815) [release/1.17] (#1841) GODRIVER-3313 [release/1.17] Skip CSOT spec tests on Windows and macOS. (#1838) GODRIVER-3358 [release/1.17] Do not override authSource from TXT record (#1840) GODRIVER-2589 [release/1.17] Clarify `*Cursor.All()` behavior in comment. (#1839) DEVPROD-10453 Use assume_role for s3 uploads [release/1.17] (#1824) (#1837) update repo metadata
GODRIVER-3156
Summary
connection.idleDeadline
toconnection.idleStart
, allowing us to calculate different idle deadlines for different purposes.Background & Motivation
Currently the Go driver never attempts to detect connections that were closed by the other side when checking out connections. As a result, it's possible to return a connection that's been closed by the other side where any read or write operations will immediately fail. Instead, we should check if the connection has been closed when checking out a connection.