/[译]Go并发代码审查

Created Sat, 11 Jun 2022 12:44:51 +0800 Modified Sun, 12 Jun 2022 23:14:36 +0800

原文:CodeReviewConcurrency

自己水平有限,仅供参考,如有错误,请指正。联系方式可评论,或者邮件 itcuihao@gmail.com

本页是对Go代码审查意见列表的补充。这个列表的目的是为了帮助在审查Go代码时发现与并发有关的bug。

你也可以只读一遍这个列表,以刷新你的记忆,并确保你知道所有这些并发性的问题。

⚠️这个页面是由社区撰写和维护的。它包括有争议的信息,可能会有误导或不正确。


不充分的同步性和竞争条件

测试

可扩展性

时间

不充分的同步性和竞争条件

从多个goroutine并发调用HTTP处理函数是否安全?

我们很容易忽视HTTP处理程序应该是线程安全的,因为它们通常不是从项目代码的任何地方明确调用的,而只是从HTTP服务器的内部调用。

未保护的变量

是否有一些不受mutex保护的字段或变量访问,其中字段或变量是一个原始的或不明确的线程安全的类型(如atomic.Value),而这个字段可以从一个并发的goroutine中更新?由于非原子的硬件写入和潜在的内存可见性问题,即使对原始变量跳过同步读取也是不安全的。

也请看典型的数据竞赛:原始的不受保护的变量

未保护的指针

一个线程安全类型的方法没有返回一个指向受保护结构的指针?这是一个微妙的bug,它导致了前一项中描述的不受保护的访问问题。例子:

type Counters struct {
	mu   sync.Mutex
	vals map[Key]*Counter
}

func (c *Counters) Add(k Key, amount int) {
    c.mu.Lock()
    defer c.mu.Unlock()
    count, ok := c.vals[k]
    if !ok {
    	count = &Counter{sum: 0, num: 0}
    	c.vals[k] = count
    }
    count.sum += amount
    count.n += 1
}

func (c *Counters) GetCounter(k Key) *Counter {
	c.mu.Lock()
	defer c.mu.Unlock()
	return c.vals[k] // BUG! 返回一个指向结构的指针,该结构必须被保护。
}

一个可能的解决方案是在GetCounter()中返回一个拷贝,而不是一个指向结构的指针。

type Counters struct {
    mu   sync.Mutex
    vals map[Key]Counter // 注意,现在我们是直接存储计数器,而不是指针。
}

...

func (c *Counters) GetCounter(k Key) (count Counter, exists bool) {
	c.mu.Lock()
	defer c.mu.Unlock()
	return c.vals[k]
}

如果有一个以上的goroutine可以更新一个sync.Map,你不会根据之前m.Load()调用的成功与否来调用m.Store()m.Delete()?换句话说,下面的代码是饶舌的。

var m sync.Map

// 可以从多个goroutine中同时调用
func DoSomething(k Key, v Value) {
	existing, ok := m.Load(k)
	if !ok {
		m.Store(k, v) // 竞争条件——两个goroutines可以并行地执行这个程序
		... 一些其他的逻辑假设`k`中的值现在是映射中的`v`
    }
    ...
}

这样的竞争条件在某些情况下可能是良性的:例如,Load()Store()调用之间的逻辑是计算要缓存在映射中的值,这种计算总是返回相同的结果,不会产生副作用。

⚠️可能会有误导性的信息。“竞赛条件 “可以指逻辑错误,就像这个例子一样,它可能是良性的。但这个短语也常用于指违反内存模型的情况,而这绝不是良性的。

如果竞赛条件不是良性的,请使用sync.Map.LoadOrStore()LoadAndDelete()方法来修复它。

扩展性

通道

创建一个零容量的通道,就像make(chan *Foo)一样,这是故意的吗?一个向零容量通道发送消息的goroutine会被阻塞,直到另一个goroutine收到这个消息。在make()调用中省略容量可能只是一个错误,这将限制代码的可扩展性,而且很可能单元测试不会发现这样的错误。

⚠️误导性的信息。缓冲通道与非缓冲通道相比,本质上没有增加 “可扩展性”。然而,缓冲通道可以很容易地掩盖死锁和其他基本的设计错误,而这些错误在非缓冲通道中会立即显现。

与普通的sync.Mutex相比,用sync.RWMutex锁定会产生额外的开销,此外,目前Go中RWMutex的实现可能存在一些扩展性问题。除非情况非常明确(比如一个RWMutex用于同步许多只读操作,每个操作持续数百毫秒或更多,而需要独占锁的写操作很少发生),否则应该有一些基准来证明RWMutex确实有助于提高性能。一个典型的例子是,RWMutex肯定是弊大于利的,那就是对一个结构中的变量的简单保护。

type Box struct {
	mu sync.RWMutex // 不要这样做 -- 利用Mutex代替
	x  int
}

func (b *Box) Get() int {
	b.mu.RLock()
	defer b.mu.RUnlock()
	return b.x
}

func (b *Box) Set(x int) {
	b.mu.Lock()
	defer b.mu.Unlock()
	b.x = x
}

时间

定时器

Time.Ticker是否使用defer tick.Stop()停止? 当循环中使用ticker的函数返回时,不停止ticker是一种内存泄漏。

时间比较

time.Time结构是否使用Equal()方法进行比较,而不仅仅是==? 引用time.Time的文档

请注意,Go ==操作符不仅要比较时间瞬间,还要比较位置和单调的时钟读数。因此,如果不首先保证所有值都设置了相同的Location(可以通过使用UTC()Local()方法来实现),并且通过设置t = t.Round(0)来剥离单调的时钟读数,则不应将时间值用作地图或数据库键。一般来说,首选t.Equal(u)而不是t == u,因为t.Equal()使用了最精确的比较,并且正确处理了只有一个参数有单调时钟读数的情况。

time.Since

在调用time.Since(t)之前,单调成分没有从t中剥离出来?这是上一条的结果。如果在调用time.Since()函数之前(通过调用UTC()Local()In()Round()Truncate()AddDate())将单调成分从time.Time结构中剥离,那么在很多情况下time.Since()的结果可能是负数,例如在最初获得起始时间和调用time.Since()的时刻之间,系统时间已经通过NTP进行了同步。如果单调成分没有被剥离,time.Since()将总是返回一个正的持续时间。

time.Before

如果你想通过t.Before(u)来比较系统时间,你是否从参数中剥离单调成分,例如通过u.Round(0)?这是与时间比较相关的另一点。有时,您需要仅通过存储在其中的系统时间来比较两个time.Time结构,具体而言。在将这些时间结构之一存储在磁盘上或通过网络发送之前,您可能需要这样做。例如,想象一下某种遥测代理,它定期将遥测指标和时间一起推送给某个远程系统。

var latestSentTime time.Time

func pushMetricPeriodically(ctx context.Context) {
	t := time.NewTicker(time.Second)
	defer t.Stop()
	for {
		select {
		case <-ctx.Done: return
		case <-t.C:
			newTime := time.Now().Round(0) // 剥离单调成分,只比较系统时间
			// 检查新的时间是否较晚,以避免在系统时间向后设置时扰乱遥测数据。
            // 在NTP同步时向后设置。
			if latestSentTime.Before(newTime) {
				sendOverNetwork(NewDataPoint(newTime, metric()))
				latestSentTime = newTime
			}
		}
	}
}

如果不调用Round(0),即剥去单调成分,这段代码就会出错。

阅读列表

Go代码审查意见:审查Go代码的检查表,不针对并发性。

Go并发性:

并发,但不是针对Go的:

Mechanical Sympathy:单写者原则