3
\$\begingroup\$

I have implemented a kind of thing that does event triggering in Golang. This is open-sourced here. Review and suggestions appreciated.

First thing I am trying to implement is adding event listeners to a trigger:

func (t *trigger) On(event string, task interface{}) error {
    if _, ok := t.functionMap[event]; ok {
        return errors.New("event already defined")
    }
    if reflect.ValueOf(task).Type().Kind() != reflect.Func {
        return errors.New("task is not a function")
    }
    t.mu.Lock()
    defer t.mu.Unlock()
    t.functionMap[event] = task
    return nil
 }

So that when Fire is called all the handler chain will be executed:

func (t *trigger) Fire(event string, params ...interface{}) ([]reflect.Value, error) {
    f, in, err := t.read(event, params...)
    if err != nil {
        return nil, err
    }
    result := f.Call(in)
    return result, nil
 }

But the thing is that don't want to maintain a whole param array for all the handlers' method parameters. How can it be done otherwise?

\$\endgroup\$
3
\$\begingroup\$

Your description of the problem indicates that each event type can have multiple listeners, but the code only implements one listener for each event. Is this intended?

The params awkwardness is a "feature" of go....

Because Go is

  • statically compiled
  • does not have function signature overloading (you can't have multiple functions with the same name, and differentiate them based on the signature)

You cannot avoid having the param input arguments to the Fire function. This is a "drawback" of Go. When you become more comfortable with the language you avoid constructs like you have, because they are hard to maintain (anything requiring reflect is probably a bad idea).

Additionally, your sync usage is broken - you need to manage the lock on both the Fire call, and the On call. Your On call is also broken because it has a race condition....

Note that reads on the map also need to be sync'd, not just writes.

So, this code:

func (t *trigger) On(event string, task interface{}) error {
    if _, ok := t.functionMap[event]; ok {
        return errors.New("event already defined")
    }
    if reflect.ValueOf(task).Type().Kind() != reflect.Func {
        return errors.New("task is not a function")
    }
    t.mu.Lock()
    defer t.mu.Unlock()
    t.functionMap[event] = task
    return nil
 }

should be:

func (t *trigger) On(event string, task interface{}) error {
    // Lock the mutex before reading it.
    t.mu.Lock()
    defer t.mu.Unlock()

    if _, ok := t.functionMap[event]; ok {
        return errors.New("event already defined")
    }
    if reflect.ValueOf(task).Type().Kind() != reflect.Func {
        return errors.New("task is not a function")
    }
    t.functionMap[event] = task
    return nil
 }

Additionally, the Fire should also have a mutex control (perhaps the t.read(...) function has a lock?)

func (t *trigger) Fire(event string, params ...interface{}) ([]reflect.Value, error) {
    f, in, err := t.read(event, params...)
    if err != nil {
        return nil, err
    }
    result := f.Call(in)
    return result, nil
 }
\$\endgroup\$
  • \$\begingroup\$ yes. t.read() has a lock inside it. One more question if i lock inside On and unlock it at defer how the t.functionMap[] = task is going to work? \$\endgroup\$ – sadlil Jul 25 '16 at 12:29
  • \$\begingroup\$ @sadlil - Your question is ... confusing. The t.mu.Lock() should happen before the t.functionMap[event] read..... and should stick around to the end of the function call so that the t.functionMap[event] = task write is in the same locked block. Your question has made me look at the ordering of the method, and you can put the "is task a function" check before the lock because that check does not depend on internal t state. \$\endgroup\$ – rolfl Jul 25 '16 at 14:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.