This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Change disk-backed storage to be atomic
ClosedPublic

Authored by kadircet on Dec 7 2018, 1:37 AM.

Details

Summary

There was a chance that multiple clangd instances could try to write
same shard, in which case we would get a malformed file most likely. This patch
changes the writing mechanism to first write to a temporary file and then rename
it to fit real destination. Which is guaranteed to be atomic by POSIX.

Diff Detail

Event Timeline

kadircet created this revision.Dec 7 2018, 1:37 AM
kadircet updated this revision to Diff 177146.Dec 7 2018, 1:45 AM
  • Fix returns

NIT: I believe we're missing a hyphen in the change description, i.e. it should be disk-backed.

clangd/index/BackgroundIndexStorage.cpp
103

Maybe extract this into a helper function?
Something like

std::error_code writeAtomically(StringRef OutPath, function_ref<void(raw_ostream&)> Writer) {
 /// create temp file, open output stream.
 Writer(OS):
 /// Move the temp file into OutPath.
}

This would keep the storeShard function as readable as it is now

105

Maybe create these temporary files in the same dir? That would ensure the move operations do not require transferring data between physical devices.
If the output file is /some/path/foo.bar, we could create files named /some/path/foo.bar.tmp#####

kadircet retitled this revision from [clangd] Change diskbackedstorage to be atomic to [clangd] Change disk-backed storage to be atomic.Dec 7 2018, 4:35 AM
kadircet updated this revision to Diff 177181.Dec 7 2018, 4:56 AM
kadircet marked 2 inline comments as done.
  • Address comments
ilya-biryukov added inline comments.Dec 13 2018, 3:38 AM
clangd/index/BackgroundIndexStorage.cpp
42

There's a helper in LLVM that will do this, llvm::createUniqueFile(), I believe it also tries multiple times in case of clashes.

55

We should not remove if no error occurred, as the same name at this point can be taken by a different action.

kadircet updated this revision to Diff 178059.Dec 13 2018, 7:10 AM
kadircet marked 2 inline comments as done.
  • Use creauteUniqueFile.
  • Don't remove on success.
ilya-biryukov accepted this revision.Dec 17 2018, 4:27 AM

LGTM with a few nits

clangd/index/BackgroundIndexStorage.cpp
39

NIT: use llvm::function_ref here. It forces to clearly state the signature of the function and allows to avoid templates.

44

NIT: I believe there's an overloaded operator +(StringRef, const char*) that would produce a Twine, so maybe simplify to OutPath + ".tmp.%%%%%"?
Just a suggestion, up to you.

60

NIT: s/it/the name/

This revision is now accepted and ready to land.Dec 17 2018, 4:27 AM
kadircet updated this revision to Diff 178448.Dec 17 2018, 4:41 AM
kadircet marked 3 inline comments as done.
  • Address comments
This revision was automatically updated to reflect the committed changes.