Golang 并发代码审查

Question

我试图了解Golang并发的最佳实践。我读了O'Reilly关于Go并发的书，然后回到了Golang Codewalks，特别是这个例子：

https://golang.org/doc/codewalk/sharemem/

这是我希望与您一起回顾的代码，以便更多地了解Go。我的第一印象是，这段代码正在破坏一些最佳实践。这当然是我（非常）没有经验的观点，我想讨论并获得对这个过程的一些见解。这不是关于谁对谁错，请好心，我只是想分享我的观点并获得一些反馈。也许这个讨论会帮助其他人理解我为什么错了，并教他们一些东西。

我完全知道这个代码的目的是教初学者，而不是成为完美的代码。

问题 1 - 无 Goroutine 清理逻辑

func main() {

    // Create our input and output channels.

    pending, complete := make(chan *Resource), make(chan *Resource)

    // Launch the StateMonitor.

    status := StateMonitor(statusInterval)

    // Launch some Poller goroutines.

    for i := 0; i < numPollers; i++ {

        go Poller(pending, complete, status)

    }

    // Send some Resources to the pending queue.

    go func() {

        for _, url := range urls {

            pending <- &Resource{url: url}

        }

    }()

    for r := range complete {

        go r.Sleep(pending)

    }

}

主要方法没有办法清理Goroutines，这意味着如果这是库的一部分，它们将被泄露。

问题 2 - 编写器未生成频道

我认为，作为最佳实践，创建、编写和清理通道的逻辑应由单个实体（或一组实体）控制。这背后的原因是，作者在写入封闭频道时会感到恐慌。因此，编写器最好创建通道，写入该通道并控制何时应关闭该通道。如果有多个写入器，则可以将它们与等待组同步。

func StateMonitor(updateInterval time.Duration) chan<- State {

    updates := make(chan State)

    urlStatus := make(map[string]string)

    ticker := time.NewTicker(updateInterval)

    go func() {

        for {

            select {

            case <-ticker.C:

                logState(urlStatus)

            case s := <-updates:

                urlStatus[s.url] = s.status

            }

        }

    }()

    return updates

}

此函数不应负责创建更新频道，因为它是频道的读取器，而不是编写器。此通道的编写者应创建它并将其传递给此函数。基本上对功能说“我会通过这个渠道将更新传递给你”。但相反，这个函数正在创建一个通道，目前还不清楚谁负责清理它。

Answer 1

这是方法，因此无需清理。返回时，程序将退出。如果不是 ，那么你是对的。mainmainmain没有适合所有用例的最佳实践。您在此处显示的代码是一种非常常见的模式。该函数创建一个 goroutine，并返回一个通道，以便其他人可以与该 goroutine 进行通信。没有规则来控制必须如何创建通道。但是，没有办法终止该goroutine。这种模式非常适合的一个用例是从数据库中读取大型结果集。该通道允许在从数据库中读取数据时对其进行流式处理。在这种情况下，通常还有其他方法可以终止 goroutine，例如传递上下文。同样，对于如何创建/关闭渠道没有硬性规定。通道可以保持打开状态，当它不再使用时，它将被垃圾回收。如果用例需要，通道可以无限期地保持打开状态，并且您担心的场景永远不会发生。

Answer 2

正如您询问此代码是否是库的一部分时，是的，在库函数内生成没有清理的goroutines将是糟糕的做法。如果这些 goroutine 执行了库的记录行为，那么调用方不知道该行为何时会发生是有问题的。如果您有任何通常“开火即忘记”的行为，则应该由呼叫者选择何时忘记它。例如：func doAfter5Minutes(f func()) {&nbsp; &nbsp;go func() {&nbsp; &nbsp; &nbsp; &nbsp;time.Sleep(5 * time.Minute)&nbsp; &nbsp; &nbsp; &nbsp;f()&nbsp; &nbsp; &nbsp; &nbsp;log.Println("done!")&nbsp; &nbsp;}()}有道理，对吧？调用该函数时，它会在 5 分钟后执行某些操作。问题是很容易像这样误用这个函数：// do the important task every 5 minutesfor {&nbsp; &nbsp; doAfter5Minutes(importantTaskFunction)}乍一看，这似乎很好。我们每5分钟做一次重要的任务，对吧？实际上，我们正在非常快速地生成许多goroutines，可能会在它们开始下降之前消耗所有可用的内存。我们可以实现某种回调或通道，以便在任务完成时发出信号，但实际上，函数应该像这样简化：func doAfter5Minutes(f func()) {&nbsp; &nbsp;time.Sleep(5 * time.Minute)&nbsp; &nbsp;f()&nbsp; &nbsp;log.Println("done!")}现在，调用方可以选择如何使用它：// call synchronouslydoAfter5Minutes(importantTaskFunction)// fire and forgetgo doAfter5Minutes(importantTaskFunction)这个功能也可以说是应该改变的。正如你所说，作者应该有效地拥有这个频道，因为他们应该是关闭它的人。事实上，这个通道读取函数坚持创建它从中读取的通道，这实际上迫使自己陷入上面提到的这种可怜的“开火和忘记”模式。请注意函数需要如何从通道读取，但它也需要在读取之前返回通道。因此，它必须将阅读行为放在一个新的，不受管理的goroutine中，以允许自己立即返回通道。func StateMonitor(updates chan State, updateInterval time.Duration) {&nbsp; &nbsp; urlStatus := make(map[string]string)&nbsp; &nbsp; ticker := time.NewTicker(updateInterval)&nbsp; &nbsp; defer ticker.Stop() // not stopping the ticker is also a resource leak&nbsp; &nbsp; for {&nbsp; &nbsp; &nbsp; &nbsp; select {&nbsp; &nbsp; &nbsp; &nbsp; case <-ticker.C:&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; logState(urlStatus)&nbsp; &nbsp; &nbsp; &nbsp; case s := <-updates:&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; urlStatus[s.url] = s.status&nbsp; &nbsp; &nbsp; &nbsp; }&nbsp; &nbsp; }}请注意，该函数现在更简单、更灵活、更同步。以前的版本真正完成的唯一一件事是，它（大多数）保证的每个实例都有一个通道，并且您不会遇到多个监视器在同一通道上竞争读取的情况。虽然这可以帮助您避免特定类别的错误，但它也使函数的灵活性大大降低，并且更有可能发生资源泄漏。StateMonitor我不确定我是否真正理解了这个例子，但通道关闭的黄金法则是，编写者应该始终负责关闭通道。请记住此规则，并注意有关此代码的几点：该方法写入Sleepr该方法同时执行，没有跟踪运行实例数量、实例处于什么状态等的方法。Sleep仅基于这些要点，我们可以说程序中可能没有任何地方可以安全关闭 ，因为似乎无法知道它是否会再次使用。r