Skip to content
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

Stop treating context errors as network errors where possible. #1045

Merged
merged 4 commits into from Aug 17, 2022
Merged

Stop treating context errors as network errors where possible. #1045

merged 4 commits into from Aug 17, 2022

Conversation

benjirewis
Copy link
Contributor

@benjirewis benjirewis commented Aug 10, 2022

GODRIVER-2468
GODRIVER-1965

Stops returning a network error when minimum RTT exceeds time remaining until deadline. Stops checking for context expiration in writeWireMessage and readWireMessage; removes associated tests. Stops checking for context expiration in WithTransaction. Starts checking for any context error before running roundTrip in Execute (including cancelation). Adds tests to ensure that Execute does not mark done contexts with the TransientTransactionError label.

Network errors can be marked with the TransientTransactionError label here, and can therefore be retryable. Context errors do not represent retryable scenarios, so we should stop calling networkError() on context errors where possible. Once we stop treating context errors as retryable, we no longer need to check explicitly for them in WithTransaction.

@benjirewis benjirewis marked this pull request as ready for review August 10, 2022 19:46
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Nicely done. This is tricky.

@kevinAlbs
Copy link
Contributor

Aside: Is there a standard term to describe when a context returns a non-nil error on Err? I sometimes read "context expiration" to mean the deadline expired or timeout has elapsed.

https://pkg.go.dev/context describes:

// WithCancel arranges for Done to be closed when cancel is called;
// WithDeadline arranges for Done to be closed when the deadline
// expires; WithTimeout arranges for Done to be closed when the timeout
// elapses.

@benjirewis benjirewis changed the title Stop treating context expiration as network error where possible. Stop treating context errors as network errors where possible. Aug 11, 2022
@benjirewis
Copy link
Contributor Author

@kevinAlbs great question. I should not have used the term "context expiration". Err returns a non-nil error when the Done channel has been closed. The error message will explain why the channel was closed, which will either be context.DeadlineExceeded or context.Canceled. The more generic term is probably "context error", as "context expiration" seems to reference context.DeadlineExceeded.

Copy link
Collaborator

@matthewdale matthewdale left a comment

Choose a reason for hiding this comment

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

Looks good 👍

You may want to add one of the GODRIVER tickets to the PR title to get the Jira integration (will make the next patch release easier because a ticket will have the automated commit messages).

x/mongo/driver/operation_test.go Outdated Show resolved Hide resolved
x/mongo/driver/operation_test.go Outdated Show resolved Hide resolved
@benjirewis
Copy link
Contributor Author

@matthewdale was planning on putting the ticket names in the body of the commit message as outlined in the onboarding doc. Jira should still pick them up.

@benjirewis benjirewis merged commit e720278 into mongodb:master Aug 17, 2022
@benjirewis benjirewis deleted the contextNetworkError.1965 branch August 17, 2022 19:15
prestonvasquez added a commit to prestonvasquez/mongo-go-driver that referenced this pull request Aug 31, 2022
prestonvasquez added a commit to prestonvasquez/mongo-go-driver that referenced this pull request Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants