ChainSafe / gossamer

🕸️ Go Implementation of the Polkadot Host
https://chainsafe.github.io/gossamer
GNU Lesser General Public License v3.0
427 stars 110 forks source link

refactor: close channel by the goroutine that write to it or don't close it #3978

Open EmilGeorgiev opened 4 months ago

EmilGeorgiev commented 4 months ago

Issue summary

Inside methods Start of metrics and pprof the channel done os closed:

go s.server.Run(ctx, ready, s.done)

select {
case <-ready:
    return nil
case err := <-s.done:
    close(s.done)
    if err != nil {
    return err
    }
    return ErrServerDoneBeforeReady
}

This is not the proper place to close the channel. In Go, it's generally recommended to close the channel by the goroutine that writes to it, especially if you want to signal to other goroutines that no more data will be sent through that channel. This helps in avoiding deadlocks and ensures that receiving goroutines can safely iterate over the channel without getting stuck indefinitely.

However, there are situations where you might not need to explicitly close the channel. If the channel's lifetime is tied to the lifetime of the program, and you know that it won't be used after a certain point, you can let it be garbage collected without closing it explicitly.

Here is probably better to remove close(s.done).

EmilGeorgiev commented 3 months ago

If you think this makes sense, can I take this issue?