Skip to content

GODRIVER-3181 Read server responses in the background after op timeout. #1719

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

Merged

Conversation

matthewdale
Copy link
Collaborator

@matthewdale matthewdale commented Jul 29, 2024

GODRIVER-3181

Summary

Read server responses in the background after op timeout.

Background & Motivation

Ports #1589 to master.

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Jul 29, 2024
@@ -1620,7 +1620,7 @@ func (op Operation) addClusterTime(dst []byte, desc description.SelectedServer)
// if the ctx is a Timeout context. If the context is not a Timeout context, it uses the
// operation's MaxTimeMS if set. If no MaxTimeMS is set on the operation, and context is
// not a Timeout context, calculateMaxTimeMS returns 0.
func (op Operation) calculateMaxTimeMS(ctx context.Context, rttMin time.Duration, rttStats string) (uint64, error) {
func (op Operation) calculateMaxTimeMS(ctx context.Context, rttMin time.Duration, rttStats string) (int64, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change maxTimeMS to use int64 since that's the wire protocol data type, preventing unnecessary conversions.

//
// Deprecated: BGReadTimeout is intended for internal use only and may be
// removed or modified at any time.
BGReadTimeout = 400 * time.Millisecond
Copy link
Collaborator Author

@matthewdale matthewdale Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a lower timeout value here (400ms) compared to v1.15 (1s) because users have observed increased database load caused by this feature in some circumstances. Reducing the timeout value reduces the amount of time that server-side operations will continue after a client-side timeout, mitigating the increased database load.

@matthewdale matthewdale force-pushed the godriver3181-port-bg-responses branch 2 times, most recently from b00d350 to 3b54b57 Compare July 29, 2024 22:43
@matthewdale matthewdale marked this pull request as ready for review July 29, 2024 22:43
prestonvasquez
prestonvasquez previously approved these changes Aug 1, 2024
@matthewdale
Copy link
Collaborator Author

I rebased this to resolve the merge conflicts.

prestonvasquez
prestonvasquez previously approved these changes Aug 6, 2024
Copy link
Contributor

API Change Report

./v2/x/mongo/driver/operation

compatible changes

(*Aggregate).OmitMaxTimeMS: added
(*Find).OmitMaxTimeMS: added

./v2/x/mongo/driver/topology

compatible changes

BGReadCallback: added
BGReadTimeout: added

prestonvasquez
prestonvasquez previously approved these changes Aug 14, 2024
@matthewdale
Copy link
Collaborator Author

Confirmed that the race detector tests currently fail intermittently in master. I'm going to merge this and investigate the race detector panics.

@matthewdale matthewdale merged commit 84650e4 into mongodb:master Aug 14, 2024
28 of 33 checks passed
@Aarif123456
Copy link

Aarif123456 commented Oct 11, 2024

@matthewdale I'm seeing panics in a Goroutine sporadically killing our program after this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-3-low Low Priority PR for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants