A patch to avoid point-time race coincidence like Rust

[Zhaoxun Yan]

A global variable has a plausible chance to race as one goroutine reads it while another writes it, as shown in previous post:

https://groups.google.com/g/golang-nuts/c/PHw_zU6Ayfo

So I am trying to enforce a lock on each global variable to avoid such accident just as Rust does on every variable.

Here is one implementation where 'S' is the global variable. You can create Int, String, Bool, ArrayInt, ArrayStr, etc in a special package, and then call its type and function/methods from anywhere. I am not sure if map type needs such special care since I heard map is okay as one goroutine reads it while another writes it at the same time.

package main

import (
    "fmt"
    "sync"
    )

type Int struct{
    mutex   sync.Mutex
    value   int
}

func (I *Int) Save(n int){
    I.mutex.Lock()
    I.value = n
    I.mutex.Unlock()
}

func (I *Int) Load() int{
    I.mutex.Lock()
    defer I.mutex.Unlock()
    return I.value
}

var S = &Int{}

func main() {   
   S.Save(5)
   fmt.Printf("S=%v, S.value=%d \n", S,  S.value)
   fmt.Println("S:", S.Load())
}

Any advice?

[Brian Candler]

On Sunday, 1 May 2022 at 07:04:34 UTC+1 yan.z...@gmail.com wrote:

Any advice?

If the value you are trying to read and update is int32 or int64, you can use the sync.Atomic package.

However, if you feel you need to do this all the time, I can't help but wonder that you're approaching Go from the wrong angle.  The solution is not to protect concurrent accesses to global variables; it is to get rid of the global variables.  Use channels instead.  Share memory by communicating; don't communicate by sharing memory.

https://www.youtube.com/watch?v=5zXAHh5tJqQ

> I am not sure if map type needs such special care since I heard map is okay as one goroutine reads it while another writes it at the same time.

No, quite the opposite: multiple goroutines accessing the same map concurrently are very likely to cause a panic.  There is sync.Map you can use safely from multiple goroutines.

[Brian Candler]

On Sunday, 1 May 2022 at 11:45:28 UTC+1 Brian Candler wrote:

get rid of the global variables.  Use channels instead.  Share memory by communicating; don't communicate by sharing memory.

https://www.youtube.com/watch?v=PAAkCSZUG1c&t=168s

http://go-proverbs.github.io/

[Zhaoxun Yan]

Hi Brian!

I just checked my previous code for race errors. It turned out that a `map` is very hard to avoid the race condition.

I have already put reading and writing into different functions with a global lock.

The reading basically sends the struct wrapped map to a channel. Then there is another goroutine processes the struct which contains the map.

The race condition did not happen between the writing function and reading function, but instead the writing function and the processing goroutine.

So that indicates neither wrapping the map inside a struct nor passing it through a channel did not make a copy of the map at all. It is the very same map that go runtime manages. It is like only the memory address of the map gets embedded in the struct and then passed over.

Do you know any way in go to copy a map? Or the only way to do it is to use json Marshal and Unmarshal?

Thanks,

   Zhaoxun

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/4AW1Ss9Tjp0/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/7b197eb6-775e-456b-abf9-30ab00becec9n%40googlegroups.com.

[Brian Candler]

Correct.

    a := make(map[int]string)
    b := a

means that b and a point to the same map.

> Do you know any way in go to copy a map? Or the only way to do it is to use json Marshal and Unmarshal?

That is called a "deep copy" and there are third party libraries for this.  But if you think you need it - just to access a map safely - then almost certainly you are Doing It Wrong™.

One option is to use sync.Map, which is safe for concurrent access.

Better: protect your map properly against concurrent access. See "Rethinking Classical Concurrency Patterns" by Bryan C. Mills - although if you're not familiar with traditional approaches to concurrency, you may find this hard to understand (it's worth the effort though)

https://www.youtube.com/watch?v=5zXAHh5tJqQ

One of the patterns which comes out of this video is to use a 1-element channel as a way to hold a protected value.  Every time you want to use this value, you pop it out of the channel, use it, and then push it back in (without keeping a copy).  This gives you the same sort of protection as a mutex, but cleaner and simpler.

Here is an example: two goroutines both concurrently updating the same map.

https://go.dev/play/p/nY3ujWh3Mfj

[Robert Engels]

You might find https://github.com/robaho/go-concurrency-test of interest. 

The channel based solution to concurrency is only suitable imo for apps with minimal concerns around its performance - ie low usage, low contention but it is often easier to get right by people not familiar with concurrent programming. 

On May 5, 2022, at 11:55 AM, Brian Candler <b.ca...@pobox.com> wrote:

Correct.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/38239c7c-2c62-45ef-85ad-3edeb26bda8bn%40googlegroups.com.

[Zhaoxun Yan]

I already know that a copy of map does not make a difference. That is why I put it inside a struct, but magic did not happen.

Does the sync.Map solves reading and writing conflict? For I checked the print out log and discovered that even `atomic` module did not prevent such race warnings.

There are also huge amount of warnings related to log4go module. But the author seemed to put a recovery patch to it,

is it another way to solve it?

https://github.com/jeanphorn/log4go/commit/7dbb8deb94686ee4c744290168390869ac014312

Although every logging of `log4go` might trigger race panic, it survived a pressure test as each incoming quotation got logged and got logged again after reading it back from redis for 8 hours, which resulted a quotation log as large as tens of MB.

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/4AW1Ss9Tjp0/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/38239c7c-2c62-45ef-85ad-3edeb26bda8bn%40googlegroups.com.

[Kurtis Rader]

On Thu, May 5, 2022 at 7:50 PM Zhaoxun Yan <yan.z...@gmail.com> wrote:

I already know that a copy of map does not make a difference. That is why I put it inside a struct, but magic did not happen.

What "magic" did you expect from simply embedding a reference to a map inside a struct? I don't ask this rhetorically since it seems like you have a fundamental misunderstanding of how Go maps and structures behave. A reference to a map is essentially a pointer to a data structure that defines the map. When you embed a map reference in a struct, or inject it into a channel, you are not making an atomic copy of the map --- you are simply copying the reference (i.e., pointer) to the map.

 

Does the sync.Map solves reading and writing conflict? For I checked the print out log and discovered that even `atomic` module did not prevent such race warnings.

See https://pkg.go.dev/sync#Map for the details but, yes, using a sync.Map "solves" concurrent reads and writes. That is, doing concurrent reads and writes on a sync.Map won't panic or leave the map in an invalid state. Whether a sync.Map is appropriate for your situation is unclear. You may need a mutex guarding the map if you need to read/write multiple keys in an atomic manner. Again, see https://pkg.go.dev/sync#Map.

Also, the `sync/atomic` module isn't what you should be using when concurrently reading and writing a map. You should be using a mutex from the `sync` module. The `sync/atomic` module is appropriate when needing to atomically modify a pointer, int32, or int64; not a map.

There are also huge amount of warnings related to log4go module. But the author seemed to put a recovery patch to it,

is it another way to solve it?

https://github.com/jeanphorn/log4go/commit/7dbb8deb94686ee4c744290168390869ac014312

Although every logging of `log4go` might trigger race panic, it survived a pressure test as each incoming quotation got logged and got logged again after reading it back from redis for 8 hours, which resulted a quotation log as large as tens of MB.

This is the first time you have mentioned the github.com/jeanphorn/log4go module and it is not clear what that module, and the commit you linked to, has to do with your original question.

--

Kurtis Rader

Caretaker of the exceptional canines Junior and Hank

[Zhaoxun Yan]

Hi Kurtis!

Thanks for your reply.

I am just stating the fact that atomic module did not solve the reading writing conflict as it may sound so, or Brian perceived so.

Here is the log analysis out of print out by a race build:

===========================

WARNING: DATA RACE
Write at 0x0000016a4808 by goroutine 91:
  sync/atomic.StoreInt64()
      /usr/go15/src/runtime/race_amd64.s:234 +0xb
  quotelive/util.Zero()
      /home/zxun/src/quotelive/util/util.go:89 +0xbe
【atomic.StoreInt64(a, 0)】
  main.startCTP()
      /home/zxun/src/quotelive/ctpschedule.go:279 +0xa5
【util.Zero(&util.CountQ)】

  github.com/robfig/cron.FuncJob.Run()
      /home/zxun/src/github.com/robfig/cron/cron.go:92 +0x34
  github.com/robfig/cron.(*Cron).runWithRecovery()
      /home/zxun/src/github.com/robfig/cron/cron.go:165 +0x72

Previous read at 0x0000016a4808 by goroutine 31:
  main.ctpDaemon()
      /home/zxun/src/quotelive/ctp.go:455 +0x257
【oldcount = util.CountQ】

Goroutine 91 (running) created at:
  github.com/robfig/cron.(*Cron).run()
      /home/zxun/src/github.com/robfig/cron/cron.go:199 +0xa90

Goroutine 31 (running) created at:
  main.initQuote()
      /home/zxun/src/quotelive/ctpschedule.go:428 +0xe4
  main.main()
      /home/zxun/src/quotelive/main.go:56 +0x251

=======================

I definitely follow the syntax of atomic while writing inside the atomic.StoreInt64 function.

But as for reading I used a simple `=`, and it obviously failed.

It seems that both reading and writing need special functions?

In that way I do not believe the atomic module makes things simpler than my proposal.

I did use sync.Map, and it did not raise such warnings. Maybe it is because I followed the strict reading and writing function syntax.

The log4go module raised lots of such race warnings, while in practice it endured high frequently logging and did not crash once.

==================
WARNING: DATA RACE
Write at 0x00c0000df7b8 by goroutine 27:
  github.com/jeanphorn/log4go.LOGGER()
      /home/zxun/src/github.com/jeanphorn/log4go/category.go:16 +0xb4
  main.pingSign()
      /home/zxun/src/quotelive/signclient.go:102 +0xc8
【log4go.LOGGER("syslog").Error("Started heartbeat to sign server")】

Previous read at 0x00c0000df7b8 by goroutine 19:
  github.com/jeanphorn/log4go.(*Filter).intLogf()
      /home/zxun/src/github.com/jeanphorn/log4go/category.go:51 +0x20f
  github.com/jeanphorn/log4go.(*Filter).Info()
      /home/zxun/src/github.com/jeanphorn/log4go/category.go:252 +0x218
  main.loginSign()
      /home/zxun/src/quotelive/signclient.go:87 +0x45e
【log4go.LOGGER("syslog").Info("Sent login message to sign server!")】
  main.connectSign()
      /home/zxun/src/quotelive/signclient.go:62 +0x544

Goroutine 27 (running) created at:
  main.loginSign()
      /home/zxun/src/quotelive/signclient.go:75 +0x78
  main.connectSign()
      /home/zxun/src/quotelive/signclient.go:62 +0x544

Goroutine 19 (finished) created at:
  main.signStart()
      /home/zxun/src/quotelive/signclient.go:30 +0x116
  main.main()
      /home/zxun/src/quotelive/main.go:53 +0x24c
==================

And here is writing vs writing race:

==================
WARNING: DATA RACE
Write at 0x00c0000df7e8 by goroutine 2:
  github.com/jeanphorn/log4go.LOGGER()
      /home/zxun/src/github.com/jeanphorn/log4go/category.go:16 +0xb4
  quotelive/myredis.Upfutureprice()
      /home/zxun/src/quotelive/myredis/connecttoredis.go:227 +0x48f
【log4go.LOGGER("marketlog").Debug("\n  " + future_id + " : " + data)】

  quotelive/ctp_market.OutputNormal.func1()
      /home/zxun/src/quotelive/ctp_market/outputQuote.go:38 +0xc4
  quotelive/ctp_market.OnQuoteGen.func1()
      /home/zxun/src/quotelive/ctp_market/main_ctp.go:213 +0x70d
  quotelive/ctp_market.OnQuote_2b()
      /home/zxun/src/quotelive/ctp_market/main_ctp.go:233 +0x50
  quotelive/ctp_market._cgoexpwrap_7d5ac618abbe_OnQuote_2b()
      _cgo_gotypes.go:321 +0x2b

Previous write at 0x00c0000df7e8 by goroutine 24:
  github.com/jeanphorn/log4go.LOGGER()
      /home/zxun/src/github.com/jeanphorn/log4go/category.go:16 +0xb4
  main.endDaemon()
      /home/zxun/src/quotelive/ctpschedule.go:314 +0xce
【log4go.LOGGER("marketlog").Debug("Snapshot time from quotation " + timestr)】

Goroutine 2 (running) created at:
  runtime.newextram()
      /usr/go15/src/runtime/proc.go:1596 +0x0

Goroutine 24 (running) created at:
  main.initQuote()
      /home/zxun/src/quotelive/ctpschedule.go:424 +0xbc
  main.main()
      /home/zxun/src/quotelive/main.go:56 +0x251
==================

[Kurtis Rader]

On Thu, May 5, 2022 at 8:50 PM Zhaoxun Yan <yan.z...@gmail.com> wrote:

Hi Kurtis!

Thanks for your reply.

I am just stating the fact that atomic module did not solve the reading writing conflict as it may sound so, or Brian perceived so.

Brian Candler wrote "If the value you are trying to read and update is int32 or int64, you can use the sync.Atomic package". You misunderstood what Brian wrote. They were not suggesting that using atomic updates were appropriate for modifying a map. They were pointing out that if you need atomic updates of ints and pointers then the `sync/atomic` package is the usual solution. In hindsight I bet Brian regrets suggesting the use of `sync/atomic`.

I definitely follow the syntax of atomic while writing inside the atomic.StoreInt64 function.

But as for reading I used a simple `=`, and it obviously failed.

Yes, that will fail because you cannot do a simple read of a value when it might be concurrently modified; even when modified by the `sync/atomic` package. You have to also use the `sync/atomic` package to atomically read the value.

 

It seems that both reading and writing need special functions?

In that way I do not believe the atomic module makes things simpler than my proposal.

I did use sync.Map, and it did not raise such warnings. Maybe it is because I followed the strict reading and writing function syntax.

Okay, if using `sync.Map` solved your problem what else do you need help with?

 

The log4go module raised lots of such race warnings, while in practice it endured high frequently logging and did not crash once.

A lot of code written in Go will emit race warnings when run with the race detector enabled. If that code explicitly captures panics then it won't crash. That is not considered a best practice. It is better to not write code that causes panics than rely on `recover()`.

[Zhaoxun Yan]

Thanks Kurtis and Brian for your discussion.

I have already decided to rewrite globals in my sync.Mutex pattern.

Unprotected global variables are threats, although at a very low possibility.

Here is the way I protect them, beyond sync.Map

----------------------------

package main

import (
    "fmt"
    "sync"
    )

type Int struct{
    mutex   sync.Mutex
    value   int
}

func (I *Int) Save(n int){
    I.mutex.Lock()
    I.value = n
    I.mutex.Unlock()
}

func (I *Int) Load() int{
    I.mutex.Lock()
    defer I.mutex.Unlock()
    return I.value

}

/*

type String struct{

...

*/

var S = &Int{}

func main() {
   
   S.Save(5)

   fmt.Printf("S=%v, S.mutex=%v, S.value=%d \n", S, S.mutex, S.value)
   fmt.Println("S:", S.Load())

}

================

S=&{{0 0} 5}, S.mutex={0 0}, S.value=5
S: 5

Good night!

  Zhaoxun