Unlike other engines, memdown stores passed Buffers by reference

[abacabadabacaba]

Most abstract-leveldown stores store the data by value. Thus, when a method such as put is called with a Buffer, they record a byte sequence that it represents and store that.

Unlike other stores, memdown stores JavaScript values as is. The problems with that is that Buffer values are mutable: their underlying memory, and thus the byte sequence that they represent, may legitimately change. In general, such changes are not expected to have a side effect of changing the contents of an abstract-memdown store, but this is what happens with memdown, because it doesn't copy the Buffer values.

Not only it makes testing more difficult (memdown cannot be used as a drop-in replacement of another store for testing), it can also result in inconsistent behavior if the ordering of keys is violated.

I think that memdown's behavior should be changed to copy Buffer values, to match the behavior of other stores. A constructor options may be provided to turn off the copying as an optimization (which will require, however, that any code using memdown does not modify the underlying memory of any passed or returned Buffer).

[vweevers]

Personally I find it acceptable, and even expected, that values in an in-memory store are mutable. That does however assume a common usage pattern, where after storing values you are done with them. I believe this is why no one has raised this issue until now. To keep performance as-is, I prefer to keep the current behavior and make copying opt-in.

I'm not sure where to solve this. We could do the copying here:

memdown/memdown.js

Lines 178 to 184 in 0f9b2e6

	MemDOWN.prototype._serializeKey = function (key) {
	return Buffer.isBuffer(key) ? key : Buffer.from(String(key))
	}
	MemDOWN.prototype._serializeValue = function (value) {
	return Buffer.isBuffer(value) ? value : Buffer.from(String(value))
	}

Along the lines of:

MemDOWN.prototype._serializeKey = function (key) {
  if (Buffer.isBuffer(key)) {
    return this._copy ? Buffer.from(key) : key
  }

  return Buffer.from(String(key))
}

However, _serializeKey is also used on range options like gt, which shouldn't be copied. So we'd have do it per method, i.e. in put() and batch(). But there we don't know whether the input came from a buffer or a string; if it was the latter we would again create unnecessary copies (effectively: Buffer.from(Buffer.from(String(key))).

[abacabadabacaba]

At least, the current behavior should be documented. Also, modifying Buffer's memory breaks snapshot guarantee, because snapshots also don't copy anything.

[vweevers]

Would you like to send a PR to document the current behavior?

[vweevers]

I documented it in https://github.com/Level/memory-level. Closing for now, until other folks share the same concern.