Ownership of signal channels is inconsistent

[jhenstridge]

At the moment, if I call Conn.RemoveSignal(ch) to stop receiving signals, the channel remains open and it is my responsibility to close it. If the connection is closed though, then godbus closes the channel (via defaultSignalHandler.Terminate()).

This is a problem for code structured like the following (error handling elided):

func handleSignals(ctx *context.Context, conn *dbus.Conn) {
    ch := make(chan *dbus.Signal)
    defer close(ch)
    conn.Signal(ch)
    defer conn.RemoveSignal(ch)

    for {
        select {
        case <-ctx.Done():
            return
        case sig := <- ch:
            // do something with the signal
        }
     }
}

If the context is done before the connection closes, then we'll clean up the channel correctly without leaking. If the connection closes first however, then we'll get a panic because the channel was closed twice.

Given that the connection can be closed outside the control of the application (e.g. if dbus-daemon drops the socket connection, then godbus will call Close() itself), it's not clear that it is possible to write correct code under these semantics.

It's not clear that this can be fixed without breaking compatibility though: if RemoveSignal() starts closing the channel, that will likely cause panics in code that expects the channel to remain open. And if Close() stops closing the channel, then it will likely lead to zombie goroutines in applications that were relying on godbus to close the channel during cleanup.

[jsouthworth]

I'd say the only sensible thing to do here would be to leave the channels open, that may cause bugs in code that expects them to be closed if the dbus connection goes away, but I would think that should be a small number of clients.

I believe (though I am not the original author) that the desired semantics here is the same as for os/signal. Keeping similar semantics here seems to make sense to me. I understand that this is breaking compatibility, but we are in a no win scenario here, it is going to have to break one way or the other because it is broken now. Something probably needs to be done to associate signals with contexts like os.NotifyContext but I'm not sure what exactly.

[guelfey]

Would there be a problem in this scenario with just not closing ch? If RemoveSignal is still called, the channel should be garbage collected even though it's still open.

[invidian]

According to https://stackoverflow.com/questions/8593645/is-it-ok-to-leave-a-channel-open, it should be fine to leave those channels opened. Perhaps what could be improved is that ownership of channels should be documented?