search

peterbourgon / diskv

A disk-backed key-value store.
http://godoc.org/github.com/peterbourgon/diskv
MIT License
1.4k stars 102 forks source link

Fix issue #63: invalid reads after write #64

Open floren opened 3 years ago

floren commented 3 years ago

Per our discussion on #62, this makes all writes atomic.

floren commented 3 years ago

PTAL. I've overhauled the way we handle atomic write temp files by jamming them into a (hidden) subdirectory. The name of this directory is configurable; anything within that directory will be ignored when listing keys.

peterbourgon commented 3 years ago

Are writes not atomic if you set TempDir to the same physical disk as the diskv path?

floren commented 3 years ago

They are. The problem is that if you don't set TempDir, ReadStream is broken. You will read inconsistent data if someone sneaks in a Write before you finish reading. So the current situation is basically "Set both BaseDir and TempDir unless you want diskv to be broken, but we won't warn you if you disregard TempDir".

I've put quite a few hours into fixing this now. If you're not interested in the fix, feel free to close the issue and we will continue to maintain and use my fork and not bother you with further PRs.

peterbourgon commented 3 years ago

Unless I fundamentally misunderstand the semantics of modern filesystems — which is possible! — an open os.File should be a snapshot of its contents when opened, and should not change if, while open, a different process or thread modifies the file on disk. Is that not true?

floren commented 3 years ago

Copy TestIssue63 from the branch into the same file in master. It should fail--it did on my Linux box, anyway. It demonstrates that what is read from an already-opened file will indeed change if the file on disk is changed.

peterbourgon commented 3 years ago

Ah, yes, I see, it's a consequence of the TempDir decisionmaking and the mode flags we pass.

Solution here is to enforce a default TempDir in the diskv path if none is provided, rather than a separate option altogether, I think.