Method has pointer receiver

[Aaron Wood]

I'm trying to understand the error I get from this and the reason why my changes fix it:

//connectors.go file

package connectors

type Connector interface {

GetDatabase() string

SetDatabase(string)

Connect() error

Ping() error

Query(string) (string, error)

}

//database.go

package database

import (

"net/url"

"fmt"

"db"

"os"

"strconv"

)

//Manages a database connection along with the required information

type Database struct {

Host     string

Port     int

Database string

Handle   *db.Client

}

//Constructor that returns a reference to a new database

func NewDatabase(db string) Database {

port, err := strconv.Atoi(os.Getenv("DB_PORT"))

if err != nil {

port = 8086

}

return Database{

os.Getenv("DB_HOST"),

port,

db,

nil,

}

}

//Gets the database being used

func (d Database) GetDatabase() string {

return d.Database

}

//Sets the database to be used

func (d *Database) SetDatabase(db string) {

d.Database = db

}

//Connects to the database and stores the handle

func (d *Database) Connect() error {

u, err := url.Parse(fmt.Sprintf("http://%s:%d", d.Host, d.Port))

if err != nil {

return err

}

conf := db.Config{

URL:      *u,

}

con, err := db.NewClient(conf)

if err != nil {

return err

}

d.Handle = con

return nil

}

//Tests the connection to the database

func (d Database) Ping() error {

d.Connect()

_, _, err := d.Handle.Ping()

if err != nil {

return err

}

return nil

}

//Queries the database and returns the results

func (d Database) Query(q string) (string, error) {

query := db.Query{

Command: q,

Database: d.Database,

}

var results string

if response, err := d.Handle.Query(query); err == nil {

if response.Error() != nil {

return results, response.Error()

}

json, err := response.MarshalJSON()

if err != nil {

return results, err

}

results = string(json)

} else {

return results, err

}

return results, nil

}

//handlers.go (this is where it's really used)

package main

import (

"net/http"

"connectors"

"connectors/database"

"github.com/gin-gonic/gin"

)

var db connectors.Connector = database.NewDatabase("")

//Determines if the database is running or not

func CheckDatabaseStatus(c *gin.Context) {

err := db.Ping()

if err != nil {

c.String(http.StatusExpectationFailed, "Can't connect to database: " + err.Error())

return

}

c.String(http.StatusOK, "Connected to database successfully")

}

The error I get when compiling this is the usual "type does not implement connectors. Connector (Connect method has pointer receiver)"

Now the fix for this is changing my database.go file like so:

//database.go

package database

import (

"net/url"

"fmt"

"db"

"os"

"strconv"

)

//Manages a database connection along with the required information

type Database struct {

Host     string

Port     int

Database string

Handle   *db.Client

}

//Constructor that returns a reference to a new database

func NewDatabase(db string) *Database {

port, err := strconv.Atoi(os.Getenv("DB_PORT"))

if err != nil {

port = 8086

}

return &Database{

os.Getenv("DB_HOST"),

port,

db,

nil,

}

}

I just don't understand why it's necessary to return a reference to Database only because something I'm implementing has a pointer receiver. Could someone please explain what's going on underneath?

[Nigel Tao]

Your method receivers have a mix of pointer and non pointer receivers.
Letting "D" stand for "Database":

func (d D) GetDatabase() etc
func (d *D) Connect() etc

Note that some of these are D, others are *D with a star.

The thing is that *D gets the methods that are defined by D, but not
vice versa. (http://golang.org/ref/spec#Method_sets says that "The
method set of the corresponding pointer type *T is the set of all
methods declared with receiver *T or T (that is, it also contains the
method set of T)")

Thus, *D has a Connect method AND a GetDatabase method (ignoring the
others for now). D without a * only has a GetDatabase method, not a
Connect method.

The Connector interface includes the Connect method. Thus, *D
satisfies this interface, but D does not.

Your original NewDatabase function returns a D, not a *D, which the
compiler correctly rejects, because a D is not a Connector.

The best fix is to make all of your receivers *D, not a mix of D and
*D. Even after that, I'd still return a *D instead of a D.

[Dustin]

The Connect() method is defined on *Database not Database, therefore *Database implements Connector, but Database doesn't. 

https://golang.org/ref/spec#Method_sets

[Roberto Zanotto]

Since you define some method on *Database and some other on Database, technically the only type that implements the interface is *Database and not Database, ergo the error in assignment.

(I'm guessing a little bit, we should check the spec)

[Aaron Wood]

Ah, thanks! That makes perfect sense. 

Why do you suggest making all of the receivers *D instead of a mix? It seems natural to me to only make methods that modify the receiver *D. I would think this would improve readability too since when other people look at it they know right away that the methods with pointer receivers do some sort of modification somewhere.

Also, why would you still return *D instead of D? What's the benefit to this if the struct isn't huge? I didn't think it'd make any sort of performance or memory difference...? 

[Nigel Tao]

On Fri, Sep 4, 2015 at 12:41 PM, Aaron Wood <aaron...@gmail.com> wrote:
> Ah, thanks! That makes perfect sense.
>
> Why do you suggest making all of the receivers *D instead of a mix? It seems
> natural to me to only make methods that modify the receiver *D.

I think it's easier if you don't think at all about which methods
modify the receiver.

For example, your code has:

func (d Database) Ping() error {
d.Connect()

etc
}

Connect modifies the receiver, but Ping's receiver is a not a pointer,
so every Ping will make a new connection (and leak it and the
underlying file descriptor when Ping returns).

> Also, why would you still return *D instead of D? What's the benefit to this
> if the struct isn't huge? I didn't think it'd make any sort of performance
> or memory difference...?

Again, returning a *D means that users of that NewD function can say:

d, err := NewD(etc)
if err != nil {
etc
}
foo(d)

and again, they don't have to think at all whether foo(d) should
really have been foo(&d), if they want to call something like d.Ping
which calls d.Connect, which modifies state.

[Aaron Wood]

Hmm, your last point is a good one. Something I'll definitely think about.

You say that I am causing a leak when calling Ping() because it calls d.Connect(). Is that because I'm calling a method that takes a pointer receiver from a method that doesn't or just because I'm overwriting the handle each time? Either way, I should really change it as it's poor design on my part :)

[Nigel Tao]

On Fri, Sep 4, 2015 at 12:55 PM, Aaron Wood <aaron...@gmail.com> wrote:
> You say that I am causing a leak when calling Ping() because it calls
> d.Connect(). Is that because I'm calling a method that takes a pointer
> receiver from a method that doesn't or just because I'm overwriting the
> handle each time? Either way, I should really change it as it's poor design
> on my part :)

It's because if you say (ignoring error handling):

d := NewD()
d.Ping()
println(d.Handle)

then you'll print nil, even though Ping calls Connect which sets the
Handle, because the receiver in "d.Ping()" is a copy of the entire d
struct, and anything that happens inside Ping only modifies that copy.
Once Ping returns, nothing in the d variable in this code fragment has
changed. If you called d.Ping a second time, you'd Connect with a
second copy, and again drop the resultant Handle.

[Aaron Wood]

Thanks, didn't realize that was the behavior when calling point receiver methods from non-pointer receiver ones. Much appreciated!

[Ben Barbour]

It's just the behaviour of calling any method with a non-pointer receiver (or a non-pointer argument, they're the same thing) - you make a copy of the type you're passing.

func foo(s *SuperBigStruct, i int) {

   // Does not make a copy of s

   // Does make a copy of i

}

func foo(s SuperBigStruct, i *int) {

   // Does make a copy of s

   // Does not make a copy of i

[Tim K]

Another reason to use pointer receivers even if the method does not modify the state of the object is when your struct is "large", you prevent copies of potentially large chunks of memory. Now, what "large" really means and at what point we should worry about it, I'm not really sure.

[James Aguilar]

I've benchmarked this before in C++ in a situation where it mattered. I was surprised at how large a struct had to be before it made sense to pass by reference. I think, on that architecture and in that program, the answer was north of 256B.

It might be harder to benchmark with Go. I don't know, but I guess the compiler might sometimes elide non-pointer receiver methods. The important thing is to benchmark where it matters and not to worry too much where it doesn't. Do the logical thing.

[Tim K]

Quoting from this page, which has lots of good tips:https://github.com/golang/go/wiki/CodeReviewComments#receiver-type

"If the receiver is a large struct or array, a pointer receiver is more efficient. How large is large? Assume it's equivalent to passing all its elements as arguments to the method. If that feels too large, it's also too large for the receiver."

Not sure that's really a good metric, e.g. if you have a struct with 20 boolean fields (or bytes, even ints) you would not want to pass them all as args to a method, but copying such a struct should not be a big deal.

The converse of that is if you have a single field, say a byte array (not slice) that's 20MB -- you would not want to copy that a lot.