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.

Sign up for GitHub

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

Jump to bottom

fix potential race condition in API client #10302

Merged
merged 6 commits into from
Apr 7, 2021
Merged

fix potential race condition in API client #10302

merged 6 commits into from
Apr 7, 2021

Conversation

cgbaker
Copy link
Contributor

@cgbaker cgbaker commented Apr 5, 2021 •
edited
Loading

Resolves #10301

Commit 4e25e8e shows a documenting test, which fails under -race:
https://app.circleci.com/pipelines/github/hashicorp/nomad/15527/workflows/d2032205-895a-4d7b-8ec6-41301a1614cb/jobs/147802

New test passes after 6064867

@cgbaker cgbaker mentioned this pull request Apr 5, 2021
Closed
@vercel vercel bot temporarily deployed to Preview – nomad April 5, 2021 22:46 Inactive
@cgbaker cgbaker force-pushed the f-10301-api-header-race-condition branch from 6fef42d to 88eb81b Compare April 5, 2021 22:46
@vercel vercel bot temporarily deployed to Preview – nomad April 5, 2021 22:46 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad April 6, 2021 00:22 Inactive
@cgbaker cgbaker changed the title documenting test for #10301 fix potential race condition in API client Apr 6, 2021
@vercel vercel bot temporarily deployed to Preview – nomad April 6, 2021 11:33 Inactive
notnoop
notnoop reviewed Apr 6, 2021
View reviewed changes
Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

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

LGTM. I suspect the significant violation here is that we are mutating the input header object, resulting into a request contaminating subsequent requests even without concurrency. The concurrency race is a consequence of that.

api/api_test.go Outdated Show resolved Hide resolved
api/api_test.go Show resolved Hide resolved
@cgbaker
Copy link
Contributor Author

cgbaker commented Apr 6, 2021 •
edited
Loading

I suspect the significant violation here is that we are mutating the input header object, resulting into a request contaminating subsequent requests even without concurrency. The concurrency race is a consequence of that.

Correct. It's not necessarily even us... the downstream code in net/http will do this as well, although this test was a short-cut to that which didn't require actually making an HTTP call.

It is possible that an auth token or another field from a request-specific QueryOptions could make it back into the config.Headers, but I wasn't able to trigger that.

@cgbaker
documenting test for #10301
4e25e8e 
enable -race detector for testing api
@cgbaker cgbaker force-pushed the f-10301-api-header-race-condition branch from 1bee19e to 4e25e8e Compare April 6, 2021 17:32
@vercel vercel bot temporarily deployed to Preview – nomad April 6, 2021 17:33 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad April 6, 2021 18:06 Inactive
@cgbaker cgbaker requested a review from notnoop April 6, 2021 18:25
notnoop
notnoop approved these changes Apr 6, 2021
View reviewed changes
Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

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

Couple of tweaks - but feel free to merge as you see fit.

.circleci/config.yml Outdated
@@ -293,11 +293,15 @@ jobs:
goarch:
type: string
default: "amd64"
enable_race_testing:
type: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be better as a boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the circleci bit, probably. however, downstream usage would require that it be converted to an empty string when false, because of the way that conditionals work in GNU make:
https://github.com/hashicorp/nomad/pull/10302/files#diff-78a8a19706dbd2a4425dd72bdab0502ed7a2cef16365ab7030a5a0588927bf47R304

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, i've changed my mind. will do that, at least for the purpose of documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@notnoop , when you have a chance, i'd appreciate your feedback on this approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

specifically, this:
https://github.com/hashicorp/nomad/pull/10302/files#diff-78a8a19706dbd2a4425dd72bdab0502ed7a2cef16365ab7030a5a0588927bf47R304

CHANGELOG.md Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – nomad April 6, 2021 20:27 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad April 6, 2021 20:39 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad April 6, 2021 21:00 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad April 6, 2021 21:30 Inactive
notnoop
notnoop approved these changes Apr 7, 2021
View reviewed changes
.circleci/config.yml
environment:
GOTEST_PKGS_EXCLUDE: "<< parameters.exclude_packages >>"
GOTEST_PKGS: "<< parameters.test_packages >>"
GOTEST_MOD: "<< parameters.test_module >>"
GOTESTARCH: "<< parameters.goarch >>"
ENABLE_RACE: "<<# parameters.enable_race_testing >>TRUE<</ parameters.enable_race_testing >>"
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL about circleci conditionals!

notnoop
notnoop reviewed Apr 7, 2021
View reviewed changes
CHANGELOG.md
@@ -17,6 +17,7 @@ IMPROVEMENTS:

BUG FIXES:
* agent: Only allow querying Prometheus formatted metrics if Prometheus is enabled within the config [[GH-10140](https://github.com/hashicorp/nomad/pull/10140)]
* api: Fixed a panic that may occur on concurrent access to an SDK client [[GH-10302](https://github.com/hashicorp/nomad/issues/10302)]
Copy link
Contributor

Choose a reason for hiding this comment

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

💯 This is even better than I was proposing about - thanks!

@cgbaker cgbaker merged commit ca66719 into main Apr 7, 2021
@cgbaker cgbaker deleted the f-10301-api-header-race-condition branch April 7, 2021 12:00
@lgfa29 lgfa29 mentioned this pull request Apr 19, 2021
Closed
@jrasell jrasell mentioned this pull request May 10, 2021
Closed
@lgfa29 lgfa29 mentioned this pull request May 10, 2021
Closed
schmichael pushed a commit that referenced this pull request May 14, 2021
@cgbaker @schmichael
Merge pull request #10302 from hashicorp/f-10301-api-header-race-cond…
f279626 
…ition

fix potential race condition in API client
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@notnoop notnoop notnoop approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Race condition in SDK client
2 participants
@cgbaker @notnoop