Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

transport: fix race accessing s.recvCompress #4627

Merged
merged 1 commit into from
Aug 3, 2021

Conversation

menghanl
Copy link
Contributor

@menghanl menghanl commented Jul 28, 2021

issue #4626

Regression caused by #3313

  • Fix on HEAD and v1.40.x will be sent separately.

RELEASE NOTES:

  • transport: fix race in transport stream accessing s.recvCompress

@menghanl menghanl added this to the 1.39 Release milestone Jul 28, 2021
@menghanl
Copy link
Contributor Author

menghanl commented Jul 28, 2021

@knz This should fix #4626. Can you give it a try? Thanks!

And what test did you run to reproduce this?
I didn't find a way to reproduce locally. It seems to only happen with a race between ctx.Cancel and header received, and I couldn't reproduce it.

@knz
Copy link

knz commented Jul 28, 2021

@menghanl thanks for the fast turnaround on this.
I won't be able to personally test this today, but I'll ask a coworker.
Otherwise I'll come back to it probably this Friday.

adityamaru added a commit to cockroachdb/vendored that referenced this pull request Jul 28, 2021
adityamaru added a commit to adityamaru/cockroach that referenced this pull request Jul 28, 2021
@adityamaru
Copy link

Hey @menghanl! Thanks for the fix, I took it for a spin in cockroachdb/cockroach#68197 and it appears to have fixed the data race.

We have a test https://github.com/cockroachdb/cockroach/blob/1d0597990de15fc7722d288a4a3ebd50cf06b50a/pkg/gossip/client_test.go#L279 which was previously failing reliably on v1.39.0 with a data race.

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

And what test did you run to reproduce this?
I didn't find a way to reproduce locally. It seems to only happen with a race
between ctx.Cancel and header received, and I couldn't reproduce it.

Fix looks good.

We really should try to find a way to reproduce this to add a regression test.

@dfawley dfawley assigned menghanl and unassigned dfawley Aug 3, 2021
@menghanl menghanl merged commit 2e0b66b into grpc:v1.39.x Aug 3, 2021
@menghanl menghanl deleted the v1.39.x_race branch August 3, 2021 17:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants