Go: How do I remove an element from a slice and modify it in memory

Question

I'm trying to write a simple server/client chat program for learning purposes but I'm stuck. I want to have the Leave function remove the pointer it gets passed and update the slice in the struct so that the pointer is no longer there. But it's not working.

Example: Input,Output

type Room struct {
    Name     string
    Visitors []*net.Conn
} 


func (r *Room) Leave(pc *net.Conn) {
    for i, pv := range r.Visitors {
        //found the connection we want to remove
        if pc == pv {
            fmt.Printf("Before %v\n",r.Visitors)
            r.Visitors = append(r.Visitors[:i], r.Visitors[i+1:]...)
            fmt.Printf("Before %v\n",r.Visitors)                
            return
        }
    }
}

The remove logic is correct (your output confirms this too). The problem might be the lack of or improper synchronization between multiple goroutines (your question says nothing about this), or there might be other issues in other parts of your code which you haven't posted. — icza, Jul 09 '16 at 10:43
It is true that there is not effort made to make sure this works with multiple goroutines but i thought, let's make sure it work with a single one and add concurrency check later when it works for one thread (maybe this is a bad mindset). I have all the code in a [gist](https://gist.github.com/Oliv95/f79c97cc7d0cba8a1fa3536a5e3caa7a) if you have the time to look — Axel Olivecrona, Jul 09 '16 at 11:37

icza · Answer 1 · 2020-01-27T12:07:36.070

The remove logic is correct (your output confirms this too). The problem is the lack of synchronization between multiple goroutines (your question says nothing about this).

You said (in your comment) that you wanted this to get working with 1 goroutine first, and deal with synchronization later. But your code already uses multiple goroutines, so you can't have that luxury:

//Let a goroutine Handle the connection
go handleConnection(conn)

Multiple goroutines are reading and writing the Room.Visitors slice, so you have no choice but to synchronize access to it.

An example would be to use sync.RWLock:

utils.go:

type Room struct {
    mu       sync.RWLock
    Name     string
    Visitors []net.Conn
}


func (r *Room) Leave(c net.Conn) {
    r.mu.Lock()
    defer r.mu.Unlock()
    for i, v := range r.Visitors {
        //found the connection we want to remove
        if c == v {
            fmt.Printf("Before remove %v\n", r.Visitors)
            r.Visitors = append(r.Visitors[:i], r.Visitors[i+1:]...)
            fmt.Printf("After remove %v\n", r.Visitors)
            return
        }
    }
}

Also whenever any other code touches Room.Visitors, also lock the code (you may use RWMutex.RLock() if you're just reading it).

Similarly you need to synchronize access to all variables that are read/changed by multiple goroutines.

Also consider zeroing the element that is freed after the removal, else the underlying array will still hold that value, preventing the garbage collector to properly free memory used by it. For more information about this, see Does go garbage collect parts of slices?

I added the mutex lock for the leave function and I'm assigning nil to the element before i remove it (i think this is what you mead by zeroing the element that is freed) but the problem is still there in a sense, i now just end up with a bunch of nil elements in my slice. [example](http://imgur.com/a/X98oo) r.Visiting[i]=nil r.Visiting = append(r.Visiting[:i],r.Visiting([i+1:]...) — Axel Olivecrona, Jul 09 '16 at 16:57
@AxelOlivecrona No. The free space after removing an element will be the last element (before removing). So to zero it, save the slice value (the header), remove the element, and zero the last value in the saved slice (assign `nil` in case of interfaces). You are not zeroing the last element, only the one being removed (and soon to be overwritten), so it has no real effect (unless the removable is the last element). — icza, Jul 09 '16 at 17:58
@AxelOlivecrona Please make sure you synchronize all other shared variable access, not just `Room.Visitors`, that was just an example. — icza, Jul 09 '16 at 17:58

score 0 · Answer 2 · edited May 23 '17 at 11:54

You are using a pointer to an interface (Visitors []*net.Conn) in your Room type. You should never need a pointer to an interface value. An interface is value is like a generic content holder and its in memory representation is different than a struct that implements that interface.

You should simply use declare the Visitors type to be an interface:

type Room struct {
    Name     string
    Visitors []net.Conn // should not be a pointer to interface value
} 


func (r *Room) Leave(pc net.Conn) { // same thing as above
    for i, pv := range r.Visitors {
        // found the connection we want to remove
        // in the comparison below actual concrete types are being compared.
        // both these concrete types must be comparable (they can't be slices for example
        if pc == pv {
            r.Visitors = append(r.Visitors[:i], r.Visitors[i+1:]...)
            return
        }
    }
}

Note that the comparision above (pc == pv) is not so trivial. Read about it here: https://golang.org/ref/spec#Comparison_operators

Also, refer to this question: Why can't I assign a *Struct to an *Interface?

The problem still persists even after the suggested change. I made a gist of all code, maybe the source of the bug is somewhere else. https://gist.github.com/Oliv95/f79c97cc7d0cba8a1fa3536a5e3caa7a — Axel Olivecrona, Jul 09 '16 at 11:30
Do the `Before remove`, `After remove` messages get printed? — abhink, Jul 09 '16 at 12:24

Go: How do I remove an element from a slice and modify it in memory

2 Answers2