Skip to content

Fixed potential torn reads in EventCounters #26630

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
Merged
halter73 merged 2 commits into from
Oct 8, 2020
Merged

Fixed potential torn reads in EventCounters #26630

halter73 merged 2 commits into from
Oct 8, 2020

Conversation

@gfoidl
Copy link
Member

@gfoidl gfoidl commented Oct 6, 2020

Fixes #22888

Used Volatile.Read.

  • x64: ordinary read as before
  • x86: Interlocked.CompareExchange(ref _loc, 0, 0) (what's actually used by Interlocked.Read)
  • arm: read with memory barrier / assume it's less "heavy" than interlocked

/cc: @halter73

@Pilchie Pilchie added community-contribution Indicates that the PR has been added by a community member area-servers labels Oct 6, 2020
@BrennanConroy
Copy link
Member

BrennanConroy commented Oct 6, 2020

FYI, you removed usings that shouldn't be removed

@JamesNK
Copy link
Member

JamesNK commented Oct 7, 2020

Re-running failing build task

@halter73 halter73 merged commit 253caf8 into dotnet:master Oct 8, 2020
@halter73
Copy link
Member

halter73 commented Oct 8, 2020

Thanks @gfoidl!

@gfoidl gfoidl deleted the eventcounters-torn-read-fix branch October 8, 2020 08:25
This was referenced Oct 8, 2020
Merged
Merged
@gfoidl
Copy link
Member Author

gfoidl commented Oct 9, 2020

FYI there was some discussion whether to use Interlocked.Read or Volatile.Read for this.

Both variants produce correct code, but the consensus is to use Interlocked.Read as it's documented to avoid torn reads, whilst for Volatile.Read it is more a side-effect because it's implemented that way.
Thus at least the docs go the route with Interlocked.Read.

I'd leave the code here as is, this is note is for completness (because the whole story started here 😉).

@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@halter73 halter73 halter73 approved these changes

@JamesNK JamesNK JamesNK approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy

@jkotalik jkotalik Awaiting requested review from jkotalik

@Tratcher Tratcher Awaiting requested review from Tratcher

Assignees

@halter73 halter73

Labels

area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Event counters need to use Interlocked.Read

6 participants

@gfoidl @BrennanConroy @JamesNK @halter73 @Pilchie @amcasey