GODRIVER-3173 Complete pending reads on conn checkout #1977
Conversation
API Change Report./v2/eventcompatible changesConnectionPendingResponseFailed: added ./v2/x/mongo/driver/topologyincompatible changesBGReadCallback: removed compatible changesPendingResponseTimeout: added |
prestonvasquez
force-pushed
the
GODRIVER-3173
branch
from
aa4debe to
a805a43
Compare
prestonvasquez
changed the title
There was a problem hiding this comment.
Pull Request Overview
This pull request implements changes to support completing pending reads on connection checkout. Key changes include:
- Adding new YAML tests for client‐side operation timeouts related to pending reads.
- Updating connection and pool logic to handle pending reads with context values (e.g. maxTimeMS and requestID) and synchronizing background reads.
- Introducing new monitoring events and updating event verification and test instrumentation to track pending read events.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| testdata/client-side-operations-timeout/pending-reads.yml | New YAML tests for handling operation timeouts and pending read scenarios. |
| internal/integration/client_test.go | Updated concurrent operation tests to account for pending reads. |
| internal/driverutil/context.go | Added context helper functions for maxTimeMS and requestID propagation. |
| x/mongo/driver/topology/pool.go | Updated pool checkout logic to await completion of pending reads and renamed timeout constants. |
| x/mongo/driver/topology/pool_test.go | Revised pool tests to verify new pending read behaviors. |
| internal/integration/unified/event_verification.go | Added fields and tests for verifying new pending read pool events. |
| event/monitoring.go | Extended pool event monitoring with pending read event support. |
| x/mongo/driver/operation.go | Propagated context values in operation execution for pending read handling. |
| internal/integration/csot_test.go | Updated CSOT tests to ensure connection closure conditions with pending reads. |
Comments suppressed due to low confidence (2)
internal/integration/unified/event_verification.go:61
- Field name 'ConnectionPendingreadSucceeded' should be 'ConnectionPendingReadSucceeded' to maintain consistent capitalization with the other pending read event fields.
ConnectionPendingreadSucceeded *struct{} `bson:"connectionPendingReadSucceeded"`
x/mongo/driver/topology/pool.go:908
- Typo in comment: 'alawys' should be corrected to 'always'. Consider removing or clarifying this inline question to avoid confusion.
if size == 0 { // Question: Would this alawys equal to zero?
There was a problem hiding this comment.
Pull Request Overview
This PR implements pending response handling by moving the pending read logic into the foreground and replacing the legacy BGReadCallback mechanism. Key changes include renaming the test function from TestBackgroundRead to TestAwaitPendingRead, replacing the awaitRemainingBytes field with a new pendingResponseState (with corresponding context values), and updating pool and connection logic to support pending response events.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| x/mongo/driver/topology/pool_test.go | Renamed test functions and updated assertions to use pendingResponseState instead of awaitRemainingBytes |
| x/mongo/driver/topology/pool.go | Replaced BGReadTimeout with PendingResponseTimeout and integrated awaitPendingResponse logic with new event publications |
| x/mongo/driver/topology/connection.go | Replaced awaitRemainingBytes with pendingResponseState and updated read methods accordingly |
| Other test and integration files | Adjusted test expectations and event verifications for pending read events |
Comments suppressed due to low confidence (1)
x/mongo/driver/topology/connection.go:929
- [nitpick] Consider removing or clarifying the inline comment about whether size would always equal zero to avoid confusion in future maintenance.
if size == 0 { // Question: Would this alawys equal to zero?
| _, err = conn.readWireMessage(ctx) | ||
| regex := regexp.MustCompile( | ||
| `^connection\(.*\[-\d+\]\) incomplete read of message header: context deadline exceeded: read tcp 127.0.0.1:.*->127.0.0.1:.*: i\/o timeout$`, | ||
| ) | ||
| assert.True(t, regex.MatchString(err.Error()), "error %q does not match pattern %q", err, regex) | ||
| assert.Nil(t, conn.awaitRemainingBytes, "conn.awaitRemainingBytes should be nil") | ||
| close(errsCh) // this line causes a double close if BGReadCallback is ever called. | ||
| assert.Nil(t, conn.pendingResponseState, "conn.awaitRemainingBytes should be nil") |
There was a problem hiding this comment.
Please update the error message to refer to 'conn.pendingResponseState' instead of 'conn.awaitRemainingBytes' for clarity and consistency with the new field name.
| assert.Nil(t, conn.pendingResponseState, "conn.awaitRemainingBytes should be nil") | |
| assert.Nil(t, conn.pendingResponseState, "conn.pendingResponseState should be nil") |
Copilot uses AI. Check for mistakes.
GODRIVER-3173
Summary
Move pending response logic into the foreground.
To test locally you will have to check out the branch here for testdata/specifications: mongodb/specifications#1675
Background & Motivation
From the specifications update: