-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
transport: hide Stream string when stored in context #1167
Conversation
The data race that's showing up:
|
ce1f88f
to
1c36b70
Compare
How about implementing func (s *Stream) GoString() string {
return fmt.Sprintf("<stream: %p, %v>", s, s.method)
} |
08f98ff
to
7e616f5
Compare
It is not thread-safe to call context.String() on any context with a stream value since valueCtx will use %#v to access all of the Stream fields without holding a lock. Instead, print the Stream's pointer and method for its GoString.
@menghanl OK, all fixed. |
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.
Thanks. LGTM.
@menghanl thank you so much for the quick review. |
@menghanl can we get a release soon? this currently blocks our CI to get things done :P |
So context.String() won't race when printing %#v. It is not thread-safe to call context.String() on any context with a stream value since valueCtx will use %#v to access all of the Stream fields without holding a lock. Instead, print the Stream's pointer and method for its GoString.
@xiang90 Just did a v1.2.1 release on v1.2.x branch. This fix in included in that release. |
It is not thread-safe to call context.String() on any context with a
stream value since valueCtx will use %#v to access all of the Stream
fields without holding a lock. Instead, use "" as the GoString.