queue icon indicating copy to clipboard operation
queue copied to clipboard

Memory leak when using method Remove()

Open Vykstorm opened this issue 2 years ago • 0 comments

While using this queue implementation in one of my projects, i noticed that the memory consumption of my process was growing indefinitely. While doing some research by profilling my application, i realized that a call to Remove() only deletes the entries of the maps Queue.items, Queue.ids for the removed element , but Queue.count value is not decreased. The effect of this is that when the Append() function is called again, Queue.count is increased, and if Queue.count >= len(Queue.buf), the buffer is increased by twice it's size.

In other words, the next code will indefinitely grow the internal buffer and consume uneeded heap memory```

q := queue.New()
for {
   q.Append(0)
   q.Remove(0)
}

I suggest an alternative implementation which fix this problem and updates Queue.buf and Queue.count properly (but with O(n) complexity).

func (q *Queue) Remove(elem interface{}) bool {
	q.mutex.Lock()
	defer q.mutex.Unlock()

	id, ok := q.ids[elem]
	if !ok {
		return false
	}
	q.moveToFront(id)

	for {
		id := q.pop()

		item, ok := q.items[id]

		if ok {
			delete(q.ids, item)
			delete(q.items, id)
			q.onChanged()
			return true
		}
	}
}

func (q *Queue) moveToFront(id int64) {
	// Precondition: id is the id of an item in the queue. 
	// Find position in buf where item with id is located
	i := q.head
	for {
		if q.buf[i] == id {
			break
		}
		i += 1
	}
	// Move elements from J=head .. i-1 to j+1
	if i == q.head {
		// Already in the front of the queue
		return
	}

	for j := i; j > q.head; j-- {
			q.buf[j] = q.buf[j-1]
	}
	
	// Place ith element into head
	q.buf[q.head] = id
}

Wrote also a unit test:


func TestRemove(t *testing.T) {
	q := New()
	k := 500
	for  i := 0; i < k; i++ {
		q.Append(i)
	}
	for i := 0; i < k; i++ {
		value := k-i-1
		q.Remove(value)

		// q.count remains consistent
		require.Equal(t, q.count, k-i-1)

		// The rest of values remains in the queue
		for j := 0; j < value; j++ {
			require.True(t, q.Contains(j))
		}
		// Queue does not contain deleted values anymore
		for j := value; j < k; j ++ {
			require.False(t, q.Contains(j))
		}
	}

	// Queue is now empty
	require.True(t, q.Empty())
}

Vykstorm avatar Apr 20 '23 13:04 Vykstorm