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

GODRIVER-2972 Fix wiremessage RequestID race in operation.Execute #1375

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

prestonvasquez
Copy link
Collaborator

GODRIVER-2972

Summary

The usage of "wiremessage.CurrentRequestID()" in the operation.Execute method will cause a race condition where concurrent calls to Execute will result in wire messages from different operations sending the same RequestID to the server.

Background & Motivation

NA

@prestonvasquez prestonvasquez requested a review from a team as a code owner September 6, 2023 02:34
@prestonvasquez prestonvasquez requested review from qingyang-hu and removed request for a team September 6, 2023 02:34
Copy link
Collaborator

@qingyang-hu qingyang-hu left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

Nice find! Looks good 👍

@@ -1120,7 +1125,7 @@ func (op Operation) createMsgWireMessage(ctx context.Context, maxTimeMS uint64,
flags |= wiremessage.ExhaustAllowed
}

info.requestID = wiremessage.CurrentRequestID()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: Are any uses of CurrentRequestID safe? Should we remove that function completely and only call NextRequestID?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good Idea, I would guess not since the requestID is a global variable. Done.

@prestonvasquez prestonvasquez temporarily deployed to api-report September 6, 2023 18:33 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Sep 6, 2023

API Change Report

./x/mongo/driver/wiremessage

incompatible changes

CurrentRequestID: removed

@prestonvasquez prestonvasquez merged commit 68bf155 into mongodb:master Sep 7, 2023
1 check passed
@prestonvasquez prestonvasquez deleted the GODRIVER-2972 branch September 7, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants