koinos / koinos-mq-golang

An MQ library used by Koinos Golang applications.
MIT License
1 stars 0 forks source link

Fatal error: Concurrent map writes #8

Closed theoreticalbts closed 3 years ago

theoreticalbts commented 3 years ago

With a long-running connection, Golang exited with fatal error: concurrent map writes. The problem is that RPCReturnMap is that SendRPC() updates the map here and here, but it's also updated here in ConsumeRPCReturnLoop().

Currently ConsumeRPCReturnLoop() function services RPC returns when acting as a client. To fix this problem, I'd suggest expanding ConsumeRPCReturnLoop() to handle SendRPC() logic as well. We should probably rename the function to RPCClientLoop() to match the expanded functionality.

The existing loop uses range to service a single channel. Since the new loop would service multiple channels, it would need to use select instead of range.

This is strictly a change to koinos-mq-golang internals. The SendRPC() interface used by callers will remain the same, however SendRPC() will now work like this:

Basically the same logic as before happens, but now all of the code that updates a consumer's RPCReturnMap happens in a single goroutine. Actually RPCReturnMap could be made a private variable of RPCClientLoop to enforce this.

Here's the backtrace:

fatal error: concurrent map writes

goroutine 33 [running]:
runtime.throw(0xce9f05, 0x15)
        /usr/lib/go-1.15/src/runtime/panic.go:1116 +0x72 fp=0xc000df72a0 sp=0xc000df7270 pc=0x437ab2
runtime.mapassign_faststr(0xbefc00, 0xc0002821b0, 0xc00069aa00, 0x20, 0xf7)
        /usr/lib/go-1.15/src/runtime/map_faststr.go:211 +0x3f1 fp=0xc000df7308 sp=0xc000df72a0 pc=0x416191
github.com/koinos/koinos-mq-golang.(*KoinosMQ).SendRPC(0xc00036c0c0, 0xce6004, 0x10, 0xce38f7, 0xc, 0xc000094b00, 0xf7, 0x100, 0xc000058c00, 0x7f32c7627108, ...)
        /opt/koinos-src/koinos-mq-golang/koinosmq.go:252 +0xbe fp=0xc000df7590 sp=0xc000df7308 pc=0x68613e
github.com/koinos/koinos-p2p/internal/rpc.(*KoinosRPC).GetBlocksByHeight(0xc000214188, 0xc000df7950, 0x371, 0xc00000000b, 0xc000df76b8, 0x4be629, 0xc000df76b8)
        /opt/koinos-src/koinos-p2p/internal/rpc/koinos_rpc.go:175 +0x156 fp=0xc000df7648 sp=0xc000df7590 pc=0x96ac96
github.com/koinos/koinos-p2p/internal/rpc.(*KoinosRPC).GetTopologyAtHeight(0xc000214188, 0x371, 0xb, 0x0, 0x0, 0x8, 0x8, 0x0, 0x0)
        /opt/koinos-src/koinos-p2p/internal/rpc/koinos_rpc.go:342 +0x23d fp=0xc000df7c98 sp=0xc000df7648 pc=0x96bcfd
github.com/koinos/koinos-p2p/internal/protocol.(*BdmiProvider).pollMyTopologyCycle(0xc0001ae100, 0xef4c40, 0xc000038048, 0xc000df7f40, 0x1, 0x0)
        /opt/koinos-src/koinos-p2p/internal/protocol/download_provider.go:296 +0x63 fp=0xc000df7f08 sp=0xc000df7c98 pc=0xa0b903
github.com/koinos/koinos-p2p/internal/protocol.(*BdmiProvider).pollMyTopologyLoop(0xc0001ae100, 0xef4c40, 0xc000038048)
        /opt/koinos-src/koinos-p2p/internal/protocol/download_provider.go:232 +0x86 fp=0xc000df7fc8 sp=0xc000df7f08 pc=0xa0b5e6
runtime.goexit()
        /usr/lib/go-1.15/src/runtime/asm_amd64.s:1374 +0x1 fp=0xc000df7fd0 sp=0xc000df7fc8 pc=0x46d081
created by github.com/koinos/koinos-p2p/internal/protocol.(*BdmiProvider).Start
        /opt/koinos-src/koinos-p2p/internal/protocol/download_provider.go:381 +0x57
theoreticalbts commented 3 years ago

Crash should be fixed by #6.

This ticket now represents refactoring to use channels / communicating sequential process instead of a mutex.

That #6 has been merged raises some questions about this refactoring:

Therefore given #6 has been merged, AFAICT the refactoring suggested in this ticket is now simply a matter of style.