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

proxy: ensure Client fields are 64-bit aligned #54

Merged
merged 2 commits into from Mar 15, 2021

Conversation

fatih
Copy link
Member

@fatih fatih commented Mar 15, 2021

fixes https://github.com/planetscale/project-big-bang/issues/135

This PR moves the connectionsCounter field to the top of the struct and ensures it's guaranteed to be 64-bit aligned.

It looks like there is a bug in Go's runtime that happens on 32-bit systems:
https://golang.org/pkg/sync/atomic/#pkg-note-BUG It happens if we use 64-bit operations on a 64-bit field. On 32-bit architectures, we need to make sure to arrange for 64-bit alignment for atomic access. There is a proposal to improve the situation: golang/go#36606

@fatih fatih requested a review from a team as a code owner March 15, 2021 09:35
fixes planetscale/surfaces#135

It looks like there is a bug in Go's runtime that happens on 32-bit
systems:
https://golang.org/pkg/sync/atomic/#pkg-note-BUG It happens if we use 64-bit
operations on 64-bit field. On 32-bit architectures, we need
to make sure to arrange for 64-bit alignment for atomic access. There is
proposal to improve the situation:
golang/go#36606

This PR moves the `connectionsCounter` field to the top of the struct
and hence makes sure it's guaranteed to be 64-bit aligned.
@fatih fatih force-pushed the fatih/fix-field-alignments branch from ad68902 to beb652c Compare March 15, 2021 09:40
@fatih fatih merged commit 68044a4 into main Mar 15, 2021
@fatih fatih deleted the fatih/fix-field-alignments branch March 15, 2021 12:24
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.

None yet

2 participants