-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
[3.4] server/embed: fix data race when start insecure grpc #15518
Conversation
There are two goroutines accessing the `gs` grpc server var. Before insecure `gs` server start, the `gs` can be changed to secure server and then the client will fail to connect to etcd with insecure request. It is data-race. We should use argument for reference in the new goroutine. fix: etcd-io#15495 Signed-off-by: Wei Fu <fuweid89@gmail.com> (cherry picked from commit a9988e2) Signed-off-by: Wei Fu <fuweid89@gmail.com>
if err == nil { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gs will not be stopped in such case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gs
is serving in the background. So I guess on etcd stop, gs will be gracefully stopped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gs will not be stopped in such case?
Currently, it won't happen. The cMux.Serve
always return error. I was thinking that we can remove that error check and just stop it. But this pr is used to fix data-race issue. It can be updated in the follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thx for the clarification.
@serathius @chaochn47 @ahrtr thanks for the review. I would like to mark it ready after we introduce the strict mode script. |
The two PRs (this one and 15519) seem unrelated. Feel free to mark it ready for review when you feel conformable. |
@ahrtr It is ready. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you @fuweid
Could you please update both 3.4 and 3.5 changelog as well? |
@ahrtr Sure. Will file pr later. Thanks for the reminder. |
There are two goroutines accessing the
gs
grpc server var. Before insecuregs
server start, thegs
can be changed to secure server and then the client will fail to connect to etcd with insecure request. It is data-race. We should use argument for reference in the new goroutine.fix: #15495
(cherry picked from commit a9988e2)
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.
cherry-pick: #15509