GODRIVER-3156 Detect and discard closed idle connections. by matthewdale · Pull Request #1815 · mongodb/mongo-go-driver

matthewdale · 2024-09-16T21:05:09Z

Summary

When checking if a connection is closed (either during check out or during the background maintenance routine), try to read from the connection to check if it's still alive.
- Only check for the liveness of the connection if it's been idle for more than 10 seconds to prevent adding latency to most check-out operations.
Change connection.idleDeadline to connection.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.

prestonvasquez · 2024-09-18T16:51:04Z

+	// 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()) {


What is the significance of 10 seconds?

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.

blink1073 · 2024-09-27T14:38:42Z

This looks relevant: x/mongo/driver/topology/pool_test.go:1230:4: undefined: noerr

mongodb-drivers-pr-bot · 2024-09-27T14:51:51Z

API Change Report

No changes found!

prestonvasquez

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
}

blink1073 · 2024-10-01T01:44:45Z

drivers-pr-bot please backport to release/1.17

Co-authored-by: Steven Silvester <steven.silvester@ieee.org> (cherry picked from commit 6952473)

…lease/1.17] (#1841) Co-authored-by: Matt Dale <9760375+matthewdale@users.noreply.github.com>

Co-authored-by: Steven Silvester <steven.silvester@ieee.org>

…ster] (#1842) Co-authored-by: Steven Silvester <steven.silvester@ieee.org>

* 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 Detect and discard closed idle connections.

d9268fd

mongodb-drivers-pr-bot Bot requested a review from prestonvasquez September 16, 2024 21:24

mongodb-drivers-pr-bot Bot added the review-priority-low Low Priority PR for Review: within 3 business days label Sep 16, 2024

prestonvasquez reviewed Sep 18, 2024

View reviewed changes

Separate liveness check method, other review changes.

39937d9

matthewdale requested a review from prestonvasquez September 26, 2024 23:09

Merge branch 'v1' into godriver3156-detect-closed-conn

ef769ea

blink1073 added review-priority-urgent High Priority PR for Review: review immediately! and removed review-priority-low Low Priority PR for Review: within 3 business days labels Sep 27, 2024

Fix missing noerr function in new pool tests.

c54db5e

prestonvasquez previously approved these changes Sep 30, 2024

View reviewed changes

Add unit tests for isAlive and add comments.

b83ad92

matthewdale dismissed prestonvasquez’s stale review via b83ad92 October 1, 2024 01:42

matthewdale requested a review from prestonvasquez October 1, 2024 01:43

prestonvasquez approved these changes Oct 1, 2024

View reviewed changes

matthewdale merged commit 6952473 into mongodb:v1 Oct 1, 2024

mongodb-drivers-pr-bot Bot pushed a commit that referenced this pull request Oct 1, 2024

GODRIVER-3156 Detect and discard closed idle connections. (#1815)

9923b8c

Co-authored-by: Steven Silvester <steven.silvester@ieee.org> (cherry picked from commit 6952473)

blink1073 pushed a commit that referenced this pull request Oct 1, 2024

GODRIVER-3156 Detect and discard closed idle connections. (#1815) [re…

b45e5d9

…lease/1.17] (#1841) Co-authored-by: Matt Dale <9760375+matthewdale@users.noreply.github.com>

matthewdale added a commit to matthewdale/mongo-go-driver that referenced this pull request Oct 1, 2024

GODRIVER-3363 Detect and discard closed idle connections. (mongodb#1815)

17b96a4

Co-authored-by: Steven Silvester <steven.silvester@ieee.org>

matthewdale mentioned this pull request Oct 1, 2024

GODRIVER-3363 Detect and discard closed idle connections. (#1815) [master] #1842

Merged

matthewdale added a commit that referenced this pull request Oct 8, 2024

GODRIVER-3363 Detect and discard closed idle connections. (#1815) [ma…

7446373

…ster] (#1842) Co-authored-by: Steven Silvester <steven.silvester@ieee.org>

mongodb-drivers-pr-bot Bot mentioned this pull request Mar 4, 2025

Merge release/1.17 into v1 #1970

Merged

mongodb-drivers-pr-bot Bot mentioned this pull request Mar 4, 2025

Merge v1 into release/2.0 #1971

Merged

This was referenced Mar 4, 2025

Merge release/2.0 into release/2.1 #1972

Merged

Merge release/2.1 into master #1973

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GODRIVER-3156 Detect and discard closed idle connections.#1815

GODRIVER-3156 Detect and discard closed idle connections.#1815
matthewdale merged 5 commits into
mongodb:v1from
matthewdale:godriver3156-detect-closed-conn

matthewdale commented Sep 16, 2024

Uh oh!

Uh oh!

prestonvasquez Sep 18, 2024

Uh oh!

matthewdale Sep 26, 2024

Uh oh!

Uh oh!

blink1073 commented Sep 27, 2024

Uh oh!

mongodb-drivers-pr-bot Bot commented Sep 27, 2024

Uh oh!

prestonvasquez left a comment

Uh oh!

blink1073 commented Oct 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

matthewdale commented Sep 16, 2024

Summary

Background & Motivation

Uh oh!

Uh oh!

prestonvasquez Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

matthewdale Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blink1073 commented Sep 27, 2024

Uh oh!

mongodb-drivers-pr-bot Bot commented Sep 27, 2024

API Change Report

Uh oh!

prestonvasquez left a comment

Choose a reason for hiding this comment

Uh oh!

blink1073 commented Oct 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants