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

grpc: prevent deadlock in Test/ClientUpdatesParamsAfterGoAway on failure #4534

Merged
merged 2 commits into from Jun 11, 2021
Merged

grpc: prevent deadlock in Test/ClientUpdatesParamsAfterGoAway on failure #4534

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c04bfb1
Prevent hang in Test/ClientUpdatesParamsAfterGoAway
dustin-ward Jun 10, 2021
42ff2ae
Move RUnlock outside of conditionals
dustin-ward Jun 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions clientconn_test.go
View file
Open in desktop
Expand Up @@ -742,6 +742,7 @@ func (s) TestClientUpdatesParamsAfterGoAway(t *testing.T) {
}
if ctx.Err() != nil {
// Timeout
cc.mu.RUnlock()
Copy link
Member

Choose a reason for hiding this comment

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

How about moving the RUnlock() to line 738 and remove the other two calls to it on 740 and 748?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It gets locked every 10ms on line 736, so I would assume you would want to unlock after each check? Moving to 730 still has the hang on failing tests

Copy link
Member

Choose a reason for hiding this comment

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

Moving to 730 still has the hang on failing tests

I suggested moving to 738 not 730. Maybe you misread?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Yes you're right, I misread. That seems like a better solution. Updated.

Copy link
Member

Choose a reason for hiding this comment

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

No worries, thank you for the fix!

t.Fatalf("cc.dopts.copts.Keepalive.Time = %v , want 20s", v)
}
cc.mu.RUnlock()
Expand Down