-
Notifications
You must be signed in to change notification settings - Fork 918
GODRIVER-3009 Fix concurrent panic in struct codec. #1477
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
Conversation
API Change ReportNo changes found! |
0153eef to
c6e1f73
Compare
This reverts commit fc9fc3c.
prestonvasquez
left a comment
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.
We need to add the following test case to TestRegistryBuilder/Register/Lookup:
{
"nil",
nil,
nil,
ErrNilType,
false,
},We might also add a test that captures the repro case in bson package:
func TestConcurrentEncoding(t *testing.T) {
t.Parallel()
wg := sync.WaitGroup{}
wg.Add(10_000)
for i := 0; i < 10_000; i++ {
go func() {
defer wg.Done()
Marshal(struct{ LastError error }{})
}()
}
wg.Wait()
}The latter might be caught by the race detector, but it couldn't hurt to have it around. What are your thoughts?
| // concurrent use by multiple goroutines after all codecs and encoders are registered. | ||
| func (r *Registry) LookupEncoder(valueType reflect.Type) (ValueEncoder, error) { | ||
| if valueType == nil { | ||
| return nil, ErrNoEncoder{Type: valueType} |
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 LookupEncoder and LookupDecoder return values should match when valueType is nil. I think it makes sense to return ErrNilType here rather than ErrNoEncoder
| return nil, ErrNoEncoder{Type: valueType} | |
| return nil, ErrNilType |
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.
I agree. However, it will be a breaking change. How do you think about filing a v2.0 ticket instead?
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 error message for ErrNilType is
"cannot perform a decoder lookup on <nil>"
We'll need to update the error message to make sense for either case.
matthewdale
left a comment
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.
Looks good 👍
Co-authored-by: Qingyang Hu <[email protected]>
Co-authored-by: Qingyang Hu <[email protected]> (cherry picked from commit 3adad2b)
GODRIVER-3009
Summary
Remove the unnecessary nil cache, so nil encoder/decoder will not be stored.
Background & Motivation
The original panic was:
from bson/bsoncodec/codec_cache.go:45
The root cause is at https://github.com/mongodb/mongo-go-driver/blob/master/bson/bsoncodec/struct_codec.go#L481