search

FactomProject / factom

Library for writing Factom clients in go
MIT License
44 stars 19 forks source link

Race condition for the APICounter #119

Closed PaulBernier closed 4 years ago

PaulBernier commented 5 years ago

The APICounter is not thread safe and generates a warning by the Golang race condition detector:

WARNING: DATA RACE
Read at 0x00c00015e678 by goroutine 32:
  github.com/FactomProject/factom.newCounter.func1()
      /home/paul/go/pkg/mod/github.com/!factom!project/factom@v0.0.0-20190712163801-e7717c4ab072/jsonrpc.go:359 +0x4a
  github.com/FactomProject/factom.GetCurrentMinute()
      /home/paul/go/pkg/mod/github.com/!factom!project/factom@v0.0.0-20190712163801-e7717c4ab072/currentminute.go:46 +0x4d

Previous write at 0x00c00015e678 by goroutine 44:
  github.com/FactomProject/factom.newCounter.func1()
      /home/paul/go/pkg/mod/github.com/!factom!project/factom@v0.0.0-20190712163801-e7717c4ab072/jsonrpc.go:359 +0x60
  github.com/FactomProject/factom.GetDBlockByHeight()
      /home/paul/go/pkg/mod/github.com/!factom!project/factom@v0.0.0-20190712163801-e7717c4ab072/dblock.go:109 +0x5b

Current implementation: https://github.com/FactomProject/factom/blob/master/jsonrpc.go#L331

It most likely has a very little or no actual impact, but having the warning show up for people using the factom lib is unnecessary noise. Golang provides out of the box some primitive for atomic counters that could be leveraged: https://gobyexample.com/atomic-counters

WhoSoup commented 5 years ago

Agreed, this is an easy fix. Currently I don't think the jsonrpc client of the factom library checks the ID field at all so this would never result in an error but if that functionality ever gets implemented, this piece won't be broken.

Submitting PR shortly