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

Fixed potential torn reads in EventCounters #22923

Merged
merged 4 commits into from
Oct 9, 2020
Merged

Fixed potential torn reads in EventCounters #22923

merged 4 commits into from
Oct 9, 2020

Conversation

gfoidl
Copy link
Member

@gfoidl gfoidl commented Oct 8, 2020

The backing fields for the counters are long, so there was potential torn read on 32-bit platforms.
Volatile.Read ensures that the value is read in a atomic way, so no torn reads can occur.

@gfoidl
Fixed potential torn reads in EventCounters
2182ac3 
The backing fields for the counters are `long`, so there was potential torn read on 32-bit platforms.
`Volatile.Read` ensures that the value is read in a atomic way, so no torn reads can occur.
@roji
Copy link
Member

roji commented Oct 8, 2020

See ongoing conversation in npgsql/npgsql#3223 (comment)

@roji roji requested review from roji and removed request for roji October 8, 2020 14:04
@roji roji self-assigned this Oct 8, 2020
@roji roji mentioned this pull request Oct 8, 2020
Merged
@gfoidl
Copy link
Member Author

gfoidl commented Oct 9, 2020

Updated to use Interlocked.Read.

@gfoidl
Fixed the build-break
2038844 
Too eagerly with find & replace -- sorry.
roji
roji reviewed Oct 9, 2020
View reviewed changes
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Thanks!

There's also a Volatile.Read and Volatile.Write in CalculateAndReset below - these should actually be a single Interlocked.Exchange operation (set to 0 and get the previous value out). Any chance you can do that as part of this PR?

@gfoidl
Copy link
Member Author

gfoidl commented Oct 9, 2020

Any chance you can do that as part of this PR?

Of course 😃 --> 2d39eb9

roji
roji approved these changes Oct 9, 2020
View reviewed changes
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Thanks!

@roji roji merged commit ffb51b6 into dotnet:main Oct 9, 2020
@gfoidl gfoidl deleted the patch-1 branch October 10, 2020 08:58
@ajcvickers
Copy link
Member

@dotnet/efteam Should we take this in 5.0?

@roji
Copy link
Member

roji commented Oct 22, 2020

I'm not sure this qualifies... It only affects 32-bit systems (really rare nowadays), and even there is really non-fatal. On the flip-side, the risk here is extremely low.

@ajcvickers
Copy link
Member

Let's discuss in triage.

@ajcvickers ajcvickers mentioned this pull request Oct 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roji roji roji approved these changes

Assignees

@roji roji

Labels
community-contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gfoidl @roji @ajcvickers