go-dbus hangs when method call generates signals

[Michael Vogt]

When you have a signal watcher and call a dbus method that generates the signal msg before the message returns it causes go-dbus to hang and never return from the method call.

Attached is a example dbus python server (to ensure its not my bad go skillz) and a sample go client that show the behavior.

I commited a fix to my branch http://bazaar.launchpad.net/~mvo/go-dbus/dbus-service/revision/136 and a testcase in r135. I used this branch because it contains all the boilerplate to create a service that triggers the hang.

[Michael Vogt (mvo)]

Here is my proposed patch. It adds a go-routine for each signalWatch. Note that the go watch.cb() referes to the go-dbus internal callback, this is not exposed publicly. The internal code that is run is sending the message to the watch.C channel.

My example go client could also be fixed by adding a go-routine for reading from watch.C. Bu tit seems to me that this is something that the library could do for me. If this is not done I think we need at least a big warning that you always need something that reads from watch.C or that it will deadlock (which honestly to me sounds like we should handle it in go-dbus instead).

[Samuele Pedroni (pedronis)]

it seems the intended usage pattern of WatchSignal was always to have a

for _ := range watch.C {
}

going on it (in some goroutine), because C has always been unbuffered

[Samuele Pedroni (pedronis)]

so client is not intended usage, the issue is that is hard to do better for go-dbus in a well-defined matter because it depends
on the pattern of signal coming,

so it's probably more a matter of documenting the behavior better

[Michael Vogt (mvo)]

Thanks for looking at this issue and for your reply!

I understand now that client is not the intended usage, I still would like to ask to consider something that makes it less likely to run into this situation.

If the reason that my initial patch is rejected is that the watchSignal() needs to run cb syncronous for names.go we could do something like in the attached patch and have a additional (internal) async callback that we can use for writing to the channel. A patch is attached that outlines what I have in mind. I think its totally fine that C is not buffered, what surprised me was that if you have nothing that consumes C via a go-routine the app will hang.

[Samuele Pedroni (pedronis)]

the problem of this kind of approaches is that if you don't set up to consume the channel eventually you will be leaking goroutines all trying to send over it which means leaking memory which also hard to track

[Samuele Pedroni (pedronis)]

also watchSignal is not exported,

the core bit is really is:

        internal, err := p.watchSignal(rule, func(msg *Message) {
                watch.C <- msg
        })

[Samuele Pedroni (pedronis)]

what I was saying is that: if that is changed to:

        internal, err := p.watchSignal(rule, func(msg *Message) {
                go func() { watch.C <- msg }
        })

we trade non-blocking with creating waiting goroutines, also we lose knowing that we get signals in order over the channel

[Samuele Pedroni (pedronis)]

so go-dbus needs better documentation about its behavior and this, but after further thinking it's unclear there's a way to do something robust and sensible without adding complexity to it, it's easier for the consumers to do the right thing, they know more about what pattern the incoming signals will have