GODRIVER-3516 Fix the issue in getOrQueueForIdleConn where connectionPerished conn.…

[linfeip]

GODRIVER-3516

…isAlive involves network requests, which can cause the p.idleMu.Lock() when obtaining conn from the pool to be blocked for too long.

Summary

the lock in getOrQueueForIdleConn will cause connection acquisition to block under high concurrency due to the IO request in c.nc.Read(b[:]) inside isAlive.

func (p *pool) getOrQueueForIdleConn(w *wantConn) bool {
        p.idleMu.Lock()
	defer p.idleMu.Unlock()

	// Try to deliver an idle connection from the idleConns stack first.
	for len(p.idleConns) > 0 {
		conn := p.idleConns[len(p.idleConns)-1]
		p.idleConns = p.idleConns[:len(p.idleConns)-1]

		if conn == nil {
			continue
		}

		if reason, perished := connectionPerished(conn); perished {
     ...
}

func (c *connection) isAlive() bool {
	if c.nc == nil {
		return false
	}
      ...
	err := c.nc.SetReadDeadline(time.Now().Add(1 * time.Millisecond))
	if err != nil {
		return false
	}
	var b [1]byte
	_, err = c.nc.Read(b[:])
	return errors.Is(err, os.ErrDeadlineExceeded)
}

Background & Motivation

Before modifying isAlive

After

[linfeip]

I think it is unnecessary to verify whether this conn is alive here, because even if the connection is alive when it is obtained, there is no guarantee that the conn will still be alive when it is passed to the caller. The connection may have been disconnected between these two steps.

[linfeip]

@prestonvasquez ping

[prestonvasquez]

@linfeip Thank you for the contribution! Would you mind also removing the connection.IsAlive() method since it's no longer being used? Otherwise this LGTM.

[mongodb-drivers-pr-bot]

API Change Report

No changes found!

[linfeip]

@linfeip Thank you for the contribution! Would you mind also removing the connection.IsAlive() method since it's no longer being used? Otherwise this LGTM.

@prestonvasquez Alright, I have removed the unused method connection.isAlive()

[qingyang-hu]

@linfeip, I appreciate your contribution! Would you mind updating the comment to match the changes? Otherwise, it looks good to me!

[linfeip]

@qingyang-hu Alright, I've already updated the comments.

[limpo1989]

Is there a plan to backport to v1.17.x ? @qingyang-hu

[prestonvasquez]

@limpo1989 This has been scheduled for the 2.2.2 and 1.17.4 patch releases.

[qingyang-hu]

👍

[qingyang-hu]

@linfeip, one more issue. Can you please remove the test case, "TestPool_checkOut/discards_connections_closed_by_the_server_side", at

mongo-go-driver/x/mongo/driver/topology/pool_test.go

Line 848 in 5214673

t.Run("discards connections closed by the server side", func(t *testing.T) {

? It is no longer needed.

[linfeip]

@linfeip, one more issue. Can you please remove the test case, "TestPool_checkOut/discards_connections_closed_by_the_server_side", at

mongo-go-driver/x/mongo/driver/topology/pool_test.go

Line 848 in 5214673

t.Run("discards connections closed by the server side", func(t *testing.T) {

? It is no longer needed.

@qingyang-hu I have removed the test cases that are no longer in use.