Possible memory leak in getMany?

[miguelmota]

Hi,

first of all thanks for providing this awesome package!

I've noticed that memory usage is creeps up higher when using getMany vs pushing to an array with get.

Examples

Using .get and pushing to an array:

const crypto = require('crypto')
const path = require('path')
const level = require('level')
const db = level(path.resolve(__dirname, './db'))
async function main() {
  for (let i = 1; i < 10; i++) {
    const keys = []
    for (let j = 0; j < 10000; j++) {
      const key = `${i}:${j}`
      keys.push(key)
      await db.put(key, crypto.randomBytes(1000))
    }
    const values = []
    for (const key of keys) {
      values.push(await db.get(key))
    }
    const mem = process.memoryUsage()
    console.log(i, Math.floor(mem.rss / 1024 / 1024))
    await new Promise((resolve) => setTimeout(() => resolve(null), 1000))
  }
}
main()

$ node index.js
1 77
2 108
3 94
4 105
5 116
6 135
7 156
8 100
9 94

Using .getMany:

const crypto = require('crypto')
const path = require('path')
const level = require('level')
const db = level(path.resolve(__dirname, './db'))
async function main() {
  for (let i = 1; i < 10; i++) {
    const keys = []
    for (let j = 0; j < 10000; j++) {
      const key = `${i}:${j}`
      keys.push(key)
      await db.put(key, crypto.randomBytes(1000))
    }
    const values = await db.getMany(keys)
    const mem = process.memoryUsage()
    console.log(i, Math.floor(mem.rss / 1024 / 1024))
    await new Promise((resolve) => setTimeout(() => resolve(null), 1000))
  }
}
main()

1 81
2 127
3 155
4 178
5 142
6 140
7 142
8 151
9 146

Running the examples multiple times shows that using getMany allocates more memory over time.

I know very little about memory leaks so let me know if I'm looking at this the wrong way! We've noticed our program eventually crashes because the memory keeps growing when using getMany (mabye) so wondering if this is expected behavior (it possibly could be something else in my application). Thanks!

[MeirionHughes]

I've had a look around. I think its the cache - it appears to be filled up with pointers.

There is a clean up if there is a non-notfound-error , but nothing to clear out the cache if its still populated by the time of the destructor

presumably the fix is

~GetManyWorker() {
    delete keys_;
  
    for (const std::string* value: cache_) {
      if (value != NULL) delete value;
    }
}

[vweevers]

We also delete it on success:

leveldown/binding.cc

Line 1215 in ba729d2

if (value != NULL) delete value;

[MeirionHughes]

missed that - hmm well no idea then. :/

[vweevers]

@miguelmota I think you should keep your test script running longer (with 100-1000 iterations instead of 10), you might see that memory usage stabilizes over time. Garbage collection isn't deterministic and it won't collect everything immediately.

[miguelmota]

I think you're right

Closing issue since it's a false lead

[vweevers]

Found a leak after all: #804