jsonpb: race marshaling logic #838
Comments
|
I can almost guarantee that the problem is that you're concurrently modifying the request message. |
dsnet
changed the title
|
Similar problem type Session struct {
}
func (session *Session) Send(msg interface{}) error {
...
session.sendChan <- msg
...
}
func (session *Session) sendLoop() {
for {
select {
case msg, ok := <-session.sendChan:
// call proto.Marshal(msg) then send the raw data
case <-session.closeChan:
return
}
}
}
...
go session.sendLoop()
...
// ProtoMessageDemo is a proto message.
type ProtoMessageDemo struct {
Data map[int32]int32
}
pmd := &ProtoMessageDemo{}
session.Send(pmd)But Hope this helps you. @neild was right. You must concurrently modify the request message. |
|
@timestee, I'm not sure how that code snippet helps us see an issue as a protobuf message (either as a |
|
@dsnet in my code snippet, |
|
Sorry, I missed that. Your code in-and-of-itself looks fine. What makes you think there is a race? |
|
|
|
The race condition @timestee presents is an expected and appropriately-caught race condition in the user’s code. Passing pointers (and pointer-semantics values) over a channel is not sharing by communicating, because both receiver and sender end up with a pointer to the same data. Modifying the same data at the same time as you are Marshaling is appropriately a simultaneous read/write violation. |
|
@puellanivis yes, i feel @srcLurker might have the same issue. Concurrently modifying the request message somewhere. |
|
Closing as obsolete. This seems to be a race in user code. Can re-open if that is not the case. |
|
I believe this is actually a real issue. Here's a repro: https://gist.github.com/riannucci/8021fcddc6ba055c2f124bb92ef51b20 Important bits:
|
|
@riannucci, thank you for your report. I am able to reproduce. However, it is a race that is unlikely to the be the same as originally reported. Probably would have been better to be a newly filed issue. Comments to closed issues have a high probability of being forgotten. |
dsnet
changed the title
|
Ah, sorry, I was just guessing that it would have the same underlying race. Should have filed a new bug and mentioned this one instead. Thanks for taking a look :) |
The reflect.Value.Interface method shallow copies the underlying value, which may copy mutexes and atomically-accessed fields. Some usages of the Interface method is only to check if the interface value implements an interface. In which case the shallow copy was unnecessary. Change those usages to use the reflect.Value.Implements method instead. Fixes #838
The reflect.Value.Interface method shallow copies the underlying value, which may copy mutexes and atomically-accessed fields. Some usages of the Interface method is only to check if the interface value implements an interface. In which case the shallow copy was unnecessary. Change those usages to use the reflect.Value.Implements method instead. Fixes #838
|
(Thanks for the fast fix; I've verified that this fixes the issue in the original codebase where we detected this) |
The reflect.Value.Interface method shallow copies the underlying value, which may copy mutexes and atomically-accessed fields. Some usages of the Interface method is only to check if the interface value implements an interface. In which case the shallow copy was unnecessary. Change those usages to use the reflect.Value.Implements method instead. Fixes #838
|
I also have this bug in table_marshal.go
the log: fatal error: concurrent map iteration and map write goroutine 43 [running]: |
|
@awinlei This is almost certainly a race condition in your code--you are probably modifying the contents of a message at the same time as marshaling it. In the event that this is not the case, the stack trace indicates that you're using an older version of the proto package. The marshal implementation has been rewritten since that version. The first step would be to try upgrading to a newer version of the package. |
Our team is using the grpc library which in turn is using this library. Occasionally we are seeing:
fatal error: concurrent map read and map writewith a stack trace (included the stack trace below). Curious if others have seen something like this.
The version we are using is from Feb 5th (
c823c79ea1570fb5ff454033735a8e68575d1d0f). Looking over the commit list, there doesn't seem to be any checkin since then that may be related, but we could have missed something.-- charles
The text was updated successfully, but these errors were encountered: