Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak in FiniteReplayProvider #22

Closed
hugowetterberg opened this issue Feb 2, 2024 · 4 comments
Closed

Memory leak in FiniteReplayProvider #22

hugowetterberg opened this issue Feb 2, 2024 · 4 comments
Labels
bug Something isn't working
Milestone

Comments

@hugowetterberg
Copy link
Contributor

The slice operations in the FiniteReplayProvider leads to an infinitely growing buffer of messages as the capacity of the buffer will increase on the first append after each dequeue. Reproduced the behaviour as a small code sample:

package main

import "fmt"

func main() {
	buf := make([]int, 0, 10)

	var (
		lastDequeue  int
		dequeueCount int
	)

	for i := 0; i < 200; i++ {
		buf = append(buf, i)

		if dequeueCount > 0 && lastDequeue == i-1 {
			println("\t- on append after dequeue:",
				"length", len(buf),
				"cap", cap(buf))
		}

		// Comparing length to cap with the assumption that it will
		// remain unchanged.
		if len(buf) == cap(buf) {
			fmt.Printf(
				"dequeue after %d iterations, length %d, cap %d\n",
				len(buf), cap(buf), 1+i-lastDequeue)

			lastDequeue = i
			dequeueCount++

			// Moving the slice ahead in backing array to dequeue,
			// length will now be one less, but next time we append
			// the capacity will increase to accommodate the new
			// item.
			buf = buf[1:]

			println("\t- after dequeue:",
				"length", len(buf),
				"cap", cap(buf))

		}
	}

	println()
	println("total dequeues:", dequeueCount)

	println("final length:", len(buf))
	println("final cap:", cap(buf))

	println()
	println("items:")

	// We will still retain all items minus the number of dequeues that have
	// been performed.
	for _, i := range buf {
		if i%10 == 0 {
			println()
		}

		fmt.Printf("%3d ", i)
	}
}

Output:

dequeue after 10 iterations, length 10, cap 10
	- after dequeue: length 9 cap 9
	- on append after dequeue: length 10 cap 18
dequeue after 18 iterations, length 18, cap 10
	- after dequeue: length 17 cap 17
	- on append after dequeue: length 18 cap 36
dequeue after 36 iterations, length 36, cap 20
	- after dequeue: length 35 cap 35
	- on append after dequeue: length 36 cap 72
dequeue after 72 iterations, length 72, cap 38
	- after dequeue: length 71 cap 71
	- on append after dequeue: length 72 cap 144
dequeue after 144 iterations, length 144, cap 74
	- after dequeue: length 143 cap 143
	- on append after dequeue: length 144 cap 288

total dequeues: 5
final length: 195
final cap: 288

items:
  5   6   7   8   9 
 10  11  12  13  14  15  16  17  18  19 
 20  21  22  23  24  25  26  27  28  29 
 30  31  32  33  34  35  36  37  38  39 
 40  41  42  43  44  45  46  47  48  49 
 50  51  52  53  54  55  56  57  58  59 
 60  61  62  63  64  65  66  67  68  69 
 70  71  72  73  74  75  76  77  78  79 
 80  81  82  83  84  85  86  87  88  89 
 90  91  92  93  94  95  96  97  98  99 
100 101 102 103 104 105 106 107 108 109 
110 111 112 113 114 115 116 117 118 119 
120 121 122 123 124 125 126 127 128 129 
130 131 132 133 134 135 136 137 138 139 
140 141 142 143 144 145 146 147 148 149 
150 151 152 153 154 155 156 157 158 159 
160 161 162 163 164 165 166 167 168 169 
170 171 172 173 174 175 176 177 178 179 
180 181 182 183 184 185 186 187 188 189 
190 191 192 193 194 195 196 197 198 199 

https://go.dev/play/p/Ut8T-tyB2-5

@tmaxmax
Copy link
Owner

tmaxmax commented Feb 2, 2024

Hi and thank you for raising the issue!

I've thought of this before and I've made the assumption that when the slice is reallocated the dequeued elements would be dropped – I think I've even left a comment in source code about it.

Given that you've flagged this problem, it's most probable I was wrong. I'll analyze your reproduction and do some research – I'll be back with a response in about 2 days.

Does this issue raise important problems for your use-case? Just trying to gauge how critical this is for you at the moment.

Again, thank you for the issue and for creating the reproduction! It will make things faster and easier. I'll be right back soon with a response and a fix.

@hugowetterberg
Copy link
Contributor Author

It’s not critical, I noticed the memory leak in a pre-production system, so it’s not really affecting anything at the moment.

I wrote a new finite replay provider that uses a circular buffer instead of the base buffer in the package. I could send it your way as a PR.

Thank you for your great work in writing and maintaining this package!

@tmaxmax
Copy link
Owner

tmaxmax commented Feb 4, 2024

A PR would be very welcome! Thank you for your work.

EDIT: PR #23 is now open.

@tmaxmax tmaxmax added this to the v1 milestone Feb 5, 2024
@tmaxmax tmaxmax added the bug Something isn't working label Feb 6, 2024
@tmaxmax
Copy link
Owner

tmaxmax commented Mar 1, 2024

Sorry for taking so long to merge #23. Released this particular update under v0.9.0-pre.1.

@tmaxmax tmaxmax closed this as completed Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants