5
\$\begingroup\$

I am in the process of learning go and am coming from a PHP, JS, and Nodejs background.

I created a package that is a client which connects to a socket server and processes the data received from the server. I am using a callback pattern and am wondering if this is frowned upon in the Go community. Here are the relevant code snippets:

// SocketClient allows handles the socket connection to a server
type SocketClient struct {
    Host string
    Path string
    conn *websocket.Conn
}

// SocketResponse function callback for data from socket
type SocketResponse func(res []byte, err error)

// Connect create the connection with the host
func (sc SocketClient) Connect(cb SocketResponse) {
    interrupt := make(chan os.Signal, 1)
    signal.Notify(interrupt, os.Interrupt)

    u := url.URL{Scheme: "wss", Host: sc.Host, Path: sc.Path}

    var err error
    sc.conn, _, err = websocket.DefaultDialer.Dial(u.String(), nil)

    if err != nil {
        cb(nil, err)
    }

    defer sc.conn.Close()

    done := make(chan struct{})

    // anonymous function call
    go func() {
        defer sc.conn.Close()
        defer close(done)
        for {
            _, message, err := sc.conn.ReadMessage()
            if err != nil {
                cb(nil, err)
            }
            cb(message, nil)
        }
    }()

    // ...
}

And then in another file I actually initialize SocketClient and call Connect...

sc.Connect(func(res []byte, err error) {
    // handle the response
})
\$\endgroup\$
3
\$\begingroup\$

I am using a callback pattern and am wondering if this is frowned upon in the Go community.

"callback pattern" shouldn't be frowned upon (they are used in "first-class functions" for instance: https://blog.golang.org/first-class-functions-in-go-and-new-go https://dave.cheney.net/2016/11/13/do-not-fear-first-class-functions)

However regarding your code, I don't think that it looks like idiomatic go code.


Don't just check errors, handle them gracefully

sc.conn, _, err = websocket.DefaultDialer.Dial(u.String(), nil)

if err != nil {
    cb(nil, err)
}

You pass the err to your callback, but you then continue inside the Connect function: if you couldn't dial, why do you continue?


Never start a goroutine without knowing how it will stop

go func() {
    defer sc.conn.Close()
    defer close(done)
    for {
        _, message, err := sc.conn.ReadMessage()
        if err != nil {
            cb(nil, err)
        }
        cb(message, nil)
    }
}()

The inner for loop will never stop (so will your goroutine).


One possibility would be to change it like this:

// SocketResponse function callback for data from socket
type SocketResponse func(res []byte)

// Connect create the connection with the host
func (sc SocketClient) Connect(cb SocketResponse) error {
    u := url.URL{Scheme: "wss", Host: sc.Host, Path: sc.Path}

    var err error
    sc.conn, _, err = websocket.DefaultDialer.Dial(u.String(), nil)

    if err != nil {
        return err
    }

    defer sc.conn.Close()
    for {
        _, message, err := sc.conn.ReadMessage()
        if err != nil {
            return err
        }
        cb(message)
    }
}

Some properties of this code :

  • on dial error, it stops
  • it reads all the messages until one error is encountered
  • it blocks (but go sc.Connect() is easy to write)

Some possible evolutions:

  • to continue even in case of errors, you could return a channel of errors (and send all errors encountered)
  • also use a channel to report the received messages
  • to control when the Connect method should stop, you could use a context or split your method in 3 parts: Connect, ReadMessage & Close
\$\endgroup\$
  • \$\begingroup\$ Thank you I really appreciate the information about the callback as well as the detailed review in general! Are you recommending to use a channel rather than a callback? If I were to send the error in the callback and then return to stop the function could I then just implement a reconnect function that has a backoff procedure and have the code that started the listening to call reconnect? Also don't I need to block on connect before listening? \$\endgroup\$ – kyle Feb 2 '18 at 16:03
  • \$\begingroup\$ Are you recommending to use a channel rather than a callback? both options are valid and kind of interchangeable (the provided callback can send the values into a channel / the goroutine receiving the values may call a callback). The drawback of the channel is the buffer size management (either the channel is given as argument, or the size of it), but its closing mechanism is a nice way to indicate the end of the messages (whereas a callback would need some additional logic). \$\endgroup\$ – oliverpool Feb 4 '18 at 20:27
  • \$\begingroup\$ If I were to send the error... it actually depends on the error: if it is an error meaning that no more Read may succeed (connexion closed for instance) in this case a new connection is needed. But if it is an error just for decoding this message (formatting or transmission error), subsequent Read should succeed. What can be done when (connect, listen, reconnect...) depends mainly on the library! \$\endgroup\$ – oliverpool Feb 4 '18 at 20:30

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.