storage: Ensure that raft methods use a consistent view of the data

[bdarnell]

Replica.Snapshot() takes a rocksdb snapshot and intends to use that
snapshot exclusively. However, it calls into other methods on the
Replica which may make their own independent accesses to
r.store.Engine() or return in-memory data. This could result in
an inconsistent view of the world and panics as in #5857.

To ensure that this can't happen, this commit moves the implementation
of Snapshot() from a method of Replica to a static function, so it
doesn't have access to anything but its rocksdb snapshot. All methods
called by snapshot() are also moved to static functions.

Fixes #5857.

---

This change is

[tbg]

LGTM. Short-lived engine snapshots are cheap, correct? Not that there's much of an alternative (except in isolated cases, and it'd be brittle).

[bdarnell]

The docs say short-lived snapshots are cheap but I haven't measured. I think internally rocksdb is doing something very close to creating a snapshot on any read. The major cost here is two extra crossings of the cgo boundary.

We might be able to get by without the snapshot if we rely on the fact that we only ever make changes on the processRaft goroutine. That's why I thought this was safe before, but I must be missing some concurrent access somewhere (and we want to improve parallelism in this code regardless).

[tbg]

Ok, was just curious. We'll see it when it shows up.

On Thu, Apr 21, 2016 at 11:06 AM Ben Darnell [email protected]
wrote:

The docs say short-lived snapshots are cheap but I haven't measured. I
think internally rocksdb is doing something very close to creating a
snapshot on any read. The major cost here is two extra crossings of the cgo
boundary.

We might be able to get by without the snapshot if we rely on the fact
that we only ever make changes on the processRaft goroutine. That's why I
thought this was safe before, but I must be missing some concurrent access
somewhere (and we want to improve parallelism in this code regardless).

—
You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#6187 (comment)

-- Tobias