Skip to content

Revisit connection pinning logic for edge cases #74

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kounat
Copy link
Contributor

@kounat kounat commented Jul 17, 2024

The new implementation with connection pinning introduces a couple of edge cases that this PR introduces a test reproduction for. Namely, the case where we're trying to concurrently execute more find queries than we have connections in the pool.

This creates a scenario where queries are stuck waiting for existing cursors to be exhausted before checking out a new connection; in the worst case, the existing cursors may time out or never close, causing the connections to stay in a checked-out state indefinitely.

We're proposing a solution below; however, this will require a change to the driver:

func (m *Mongo) RoundTrip(msg *Message, tags []string) (_ *Message, err error) {
	// ...

        if requestCursorID != 0 {
		var ok bool

		conn, ok = m.cursors.peek(requestCursorID, collection)
		if ok {
			m.log.Debug("Cached cursorID has been found", zap.Int64("cursor", requestCursorID), zap.String("collection", collection))
		}
		// Check the specific connection out of the pool -- if it is already checked out or in use, block until connection is ready.
		// We will need to implement the below function in the driver to support this.
		conn.Open()
	}

	// ...

	defer func() {
		// Return the connection back to the pool after each operation to allow for reuse by concurrent requests.
		if err := conn.Close(); err != nil {
			m.log.Error("Error closing Mongo connection", zap.Error(err), zap.String("address", addr.String()))
		}
	}()

	// ...
}

Right now there isn't an Open() function in driver.Connection interface that we can leverage to check out the same connection for a cursorID -- the existing checkoutConnection function in driver.Server only takes in a context and returns a random connection.

This solution will mimic the old implementation's logic, where connections are checked in and out after each operation and shared amongst queries. This way we won't have to exhaust the pool with checked out connections and have queries wait until entire find operations are finished.

@prestonvasquez
Copy link
Contributor

@kounat I'm wondering if we can reserve a set of connections specifically for cursors, ensuring that there are always open connections for non-cursor operations. Here is a POC: #75

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants