Extending Watcher to be able to reuse its connection

[koiuo]

It is typical to get some info from MPD only in response to some event that occurs in MPD.
For instance, I want to notify a client about song change, so I create a Watcher, and when I get reply to idle request, I ask mpd about status and current song.

To achieve this now, I need to obtain 2 connections to mpd: one from Watcher, another from a simple Client.

There is no issue with the Watcher. Client though gets disconnected after some timeout, so one needs to bother with keeping connection alive. Thus, such simple task become more complicated then it should.

All this would be much simpler, if there was a way to reuse Watcher connection.

My solution to this is to make Watcher to provide untyped events instead of name of subsystem.

My code works for me, but errors are not handled yet. I can improve that if you decide, that it might be worth to merge the feature. I'm open to any comments and suggestions.

And thanks for a great lib.

[fhs]

I don't understand why EventBuilder has to return interface{}. Whatever it returns is sent back to the user in the Event chan. Gompd doesn't need to be the middle-man, since the user already knows the subsystem name (as first argument of EventBuilder).

I think one possibility is to remove Event and Error channels from Watcher, and switch to the callback approach that you've taken. The callback would have this type:

type EventFunc func(subsystem string, err error, conn *Client)

where subsystem is set to empty string if err is non-nil.

Exporting Conn in Watcher is probably not a good idea since MPD closes the connection if any command besides noidle is executed while in idle state. The callback is guaranteed to be called while not in idle state. This doesn't prevent the user from accessing conn outside of the callback but it at least makes it clear that conn is specific to the callback.

It's also possible to use an interface like Go's http.Handler instead of a callback function, but I'm not sure it makes sense here.

@ushis is the original author of the watcher. He might have some thoughts.

[koiuo]

I agree, that exposing conn is not an option.

I'm pretty new to go. For me it seemed, that keeping current approach with channel embraces to write loosely coupled code. It suggests, that EventBuilder is just a builder and it shouldn't invoke any long-running applications. So, that was just a matter of preference.

Anyway, I agree, that callback might be enough. We just have to state explicitly in docs, that it should return as quickly as possible, otherwise, we may experience connection timeout or miss some updates from mpd (aren't we?).

Moreover, adding callback feature allows to keep current API unchanged. Let me prepare another POC.

[koiuo]

Hm... I missed, that if we have callback, channel becomes unnecessary.
And you know, I really like channel approach, because client code is very straightforward — just iterate over channel. Callbacks won't give us the same beaty.

We have an option to make it channel of *Event

type Event struct {
  Subsystem string
  Payload interface{}
}

But that will break API completely. So, frankly, I still think, having channel of interface{} is better.

[fhs]

Let's not worry about breaking API just yet. We can always add a FancyWatcher type (or whatever we decide to call it).

How are you using the watcher? It'll be easier to design the API if we know the use cases.

[koiuo]

I'm doing yet another last.fm scrobbler. Yet not ready to push it to public.

Watcher is wrapped into another type, that handles auto-reconnections in background goroutine, and provides error-free channel of typed events.

With some simplification, it is like this

func eventBuilder(subsystem string, conn *mpd.Client) (response interface{}, err error) {
    status, err := conn.Status()
    if err != nil {
        return
    }
    song, err := conn.CurrentSong()
    if err != nil {
        return
    }
    response = []interface{}{status, song}
    return
}

...
func main() {
        ...

    for event := range mpdSource.Event {
        stateMachine.Consume(event)
    }
}

[fhs]

You can do something like this, sending the events to a chan inside the callback function:

type Event struct {
    status, song mpd.Attrs
}

func eventFuncBuilder(eventc chan<- Event) EventFunc {
    return func(string, err error, conn *mpd.Client) {
        if err != nil {
            return
        }
        status, err := conn.Status()
        if err != nil {
                    return
        }
        song, err := conn.CurrentSong()
        if err != nil {
            return
        }
        eventc <- Event{status, song}
    }
}

func main() {
    var eventc chan Event

    w, err := NewWatcher("tcp", ":6600", "", eventFuncBuilder(eventc))
    if err != nil {
        panic(err)
    }
    defer w.Close()

    for ev := range eventc {
        stateMachine.Consume(ev)
    }
}

I think the callback approach works well but I'm open to other approaches.

One danger of exposing the connection is that if the callback itself generates events, we can get into an infinite loop generating and consuming the same events. Perhaps the Client method set should be limited to only those methods that doesn't generate any events (Status, CurrentSong, etc.) using an interface, but that's probably overkill.

[koiuo]

Thanks for the code. I had something like this in my mind also. I understand, that it's not a big difference from what I have. I'm just emotionally stick to current approach :)

One danger of exposing the connection is that if the callback itself generates events, we can get into an infinite loop generating and consuming the same events. Perhaps the Client method set should be limited to only those methods that doesn't generate any events (Status, CurrentSong, etc.) using an interface, but that's probably overkill.

It would be the same with 2 independent connections — bare one and embedded in watcher. So I believe callback doesn't contribute much to the problem you described.

The only issue I see here, is that we may skip notifications while we're in callback. And that's the only concern I have regarding this feature. So I even doubt now, if it worth to add it.

On one hand, it seems quite natural to reuse connection, but on the other, it may produce unwanted effects and issues that even depend on machine performance where app is running (on one machine callback may appear slow enough to miss notifications, while on the other it may be ok). Correct me please, if I'm wrong.

Returning to the problem described in the first post I see 2 alternative options:

1. allow users to easily create a connection along with background go routine to keep it alive
2. make idle and noidle functions public, so watcher functionality can be reimplemented on low level by the user. And here user may decide whether he's ok with possible loss of notifications from mpd.

What do you think?

[fhs]

Thanks for the code. I had something like this in my mind also. I understand, that it's not a big difference from what I have. I'm just emotionally stick to current approach :)

An important difference is that that my proposal doesn't require using interface{} and thus no type assertions, which are not checked at compile time. That said, I'm having second thoughts about using a callback instead of an interface. Something like this:

type Handler interface {
    HandleError(err error, conn *Client)
    HandleEvent(subsystem string, conn *Client)
}

I need to think about this some more. Ultimately we want to implement Watcher using the FancyWatcher (let's call it ActiveWatcher).

The only issue I see here, is that we may skip notifications while we're in callback.

I don't think this is an issue. From http://comments.gmane.org/gmane.comp.audio.musicpd.devel/311

> Lets say I am running the idle command. So I receive events. Now
> lets say I want to pause the current song. I then have to quit the
> idle state. Send the pause command. And then call idle again. Now
> lets say in the mean time an event has been send that (should) tell
> me the play list is updated. I missed that. Tons of other examples
> can be found.

The "idle" command handles this in a safe manner: it manages an
"idle_flags" bit set for every client (see src/client.c, struct
client), and ORs each event into it, even if the client is not in
"idle" mode.  The next "idle" will return immediately, no event is
lost.  Yeah, this important detail is missing in the docs.