[PATCH] D36397: [clangd] Fixed a data race.
ilya-biryukov added a comment. Thanks. I'll make sure to address the awkwardness in further commits. https://reviews.llvm.org/D36397 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36397: [clangd] Fixed a data race.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL310818: [clangd] Fixed a data race. (authored by ibiryukov).
Repository:
rL LLVM
https://reviews.llvm.org/D36397
Files:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/ClangdUnit.h
Index: clang-tools-extra/trunk/clangd/ClangdUnit.h
===
--- clang-tools-extra/trunk/clangd/ClangdUnit.h
+++ clang-tools-extra/trunk/clangd/ClangdUnit.h
@@ -151,9 +151,17 @@
CppFile(CppFile const &) = delete;
CppFile(CppFile &&) = delete;
- /// Cancels all scheduled rebuilds and sets AST and Preamble to nulls.
+ /// Cancels a scheduled rebuild, if any, and sets AST and Preamble to nulls.
/// If a rebuild is in progress, will wait for it to finish.
- void cancelRebuilds();
+ void cancelRebuild();
+
+ /// Similar to deferRebuild, but sets both Preamble and AST to nulls instead
+ /// of doing an actual parsing. Returned future is a deferred computation that
+ /// will wait for any ongoing rebuilds to finish and actually set the AST and
+ /// Preamble to nulls. It can be run on a different thread.
+ /// This function is useful to cancel ongoing rebuilds, if any, before
+ /// removing CppFile.
+ std::future deferCancelRebuild();
/// Rebuild AST and Preamble synchronously on the calling thread.
/// Returns a list of diagnostics or a llvm::None, if another rebuild was
Index: clang-tools-extra/trunk/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdUnit.cpp
+++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp
@@ -711,10 +711,12 @@
ASTFuture = ASTPromise.get_future();
}
-void CppFile::cancelRebuilds() {
+void CppFile::cancelRebuild() { deferCancelRebuild().get(); }
+
+std::future CppFile::deferCancelRebuild() {
std::unique_lock Lock(Mutex);
// Cancel an ongoing rebuild, if any, and wait for it to finish.
- ++this->RebuildCounter;
+ unsigned RequestRebuildCounter = ++this->RebuildCounter;
// Rebuild asserts that futures aren't ready if rebuild is cancelled.
// We want to keep this invariant.
if (futureIsReady(PreambleFuture)) {
@@ -725,12 +727,28 @@
ASTPromise = std::promise();
ASTFuture = ASTPromise.get_future();
}
- // Now wait for rebuild to finish.
- RebuildCond.wait(Lock, [this]() { return !this->RebuildInProgress; });
- // Return empty results for futures.
- PreamblePromise.set_value(nullptr);
- ASTPromise.set_value(std::make_shared(llvm::None));
+ Lock.unlock();
+ // Notify about changes to RebuildCounter.
+ RebuildCond.notify_all();
+
+ std::shared_ptr That = shared_from_this();
+ return std::async(std::launch::deferred, [That, RequestRebuildCounter]() {
+std::unique_lock Lock(That->Mutex);
+CppFile *This = &*That;
+This->RebuildCond.wait(Lock, [This, RequestRebuildCounter]() {
+ return !This->RebuildInProgress ||
+ This->RebuildCounter != RequestRebuildCounter;
+});
+
+// This computation got cancelled itself, do nothing.
+if (This->RebuildCounter != RequestRebuildCounter)
+ return;
+
+// Set empty results for Promises.
+That->PreamblePromise.set_value(nullptr);
+That->ASTPromise.set_value(std::make_shared(llvm::None));
+ });
}
llvm::Optional
@@ -767,6 +785,8 @@
this->ASTFuture = this->ASTPromise.get_future();
}
} // unlock Mutex.
+ // Notify about changes to RebuildCounter.
+ RebuildCond.notify_all();
// A helper to function to finish the rebuild. May be run on a different
// thread.
@@ -916,7 +936,10 @@
if (WasCancelledBeforeConstruction)
return;
- File.RebuildCond.wait(Lock, []() { return !File.RebuildInProgress; });
+ File.RebuildCond.wait(Lock, [, RequestRebuildCounter]() {
+return !File.RebuildInProgress ||
+ File.RebuildCounter != RequestRebuildCounter;
+ });
WasCancelledBeforeConstruction = File.RebuildCounter != RequestRebuildCounter;
if (WasCancelledBeforeConstruction)
Index: clang-tools-extra/trunk/clangd/ClangdServer.h
===
--- clang-tools-extra/trunk/clangd/ClangdServer.h
+++ clang-tools-extra/trunk/clangd/ClangdServer.h
@@ -233,6 +233,13 @@
std::string dumpAST(PathRef File);
private:
+ std::future
+ scheduleReparseAndDiags(PathRef File, VersionedDraft Contents,
+ std::shared_ptr Resources,
+ Tagged TaggedFS);
+
+ std::future scheduleCancelRebuild(std::shared_ptr Resources);
+
GlobalCompilationDatabase
DiagnosticsConsumer
FileSystemProvider
Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===
---
[PATCH] D36397: [clangd] Fixed a data race.
bkramer accepted this revision. bkramer added a comment. This revision is now accepted and ready to land. I think this can be committed now. It's still a bit awkward but we can address that later. https://reviews.llvm.org/D36397 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36397: [clangd] Fixed a data race.
ilya-biryukov added a comment. A follow-up after discussion offline: we should make an attempt to simplify the threading code in `ClangdUnit.h` by moving the async worker loop of `CppFile` into the class itself, rather than relying on external scheduling to do the right thing. @klimek, are there any other issues we need to address in this specific patch or are we good to go with submitting it? https://reviews.llvm.org/D36397 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36397: [clangd] Fixed a data race.
ilya-biryukov added inline comments.
Comment at: clangd/ClangdUnit.cpp:714
-void CppFile::cancelRebuilds() {
+void CppFile::cancelRebuild() { deferCancelRebuild().get(); }
+
klimek wrote:
> Somewhat orthogonal question - why CppFile? We do support other languages in
> Clangd, right? (C / Obj-C)
> I'd have expected this to be called ClangdUnit, especially given the file
> name.
Just wanted to give it a different name when the class changed semantics.
Didn't rename the file yet, though, because was afraid that git would not
detect renames and we'll lose history.
ClangdUnit used to provide implementations for all features (codeComplete,
etc.), CppFile manages AST and Preamble., i.e. doing a different thing.
But I agree, CppFile is a probably a bad name, but I would still go with
something not ending with Unit to not bring up parallels with ASTUnit, that
provides a very different interface.
How about ClangdFile, ClangdFileAST?
Comment at: clangd/ClangdUnit.cpp:735
+
+ std::shared_ptr That = shared_from_this();
+ return std::async(std::launch::deferred, [That, RequestRebuildCounter]() {
klimek wrote:
> Now I know why I don't like the shard_ptr, this is all pretty awkward.
Do you mean `shared_from_this()` specifically?
Comment at: clangd/ClangdUnit.cpp:738
+std::unique_lock Lock(That->Mutex);
+CppFile *This = &*That;
+This->RebuildCond.wait(Lock, [This, RequestRebuildCounter]() {
klimek wrote:
> Why do we need that instead of just calling the bound shared_ptr This?
This lambda is a deferred computation and might get executed on a different
thread, so we want to keep a `shared_ptr` to our class around (`weak_ptr` would
also work) to make sure this class does not get deleted before this lambda is
called.
`This` local var is for capture in a lambda that is passed to `wait` in the
code below (we could've captured `That`, but that would be an extra
`shared_ptr` copy).
https://reviews.llvm.org/D36397
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36397: [clangd] Fixed a data race.
klimek added inline comments.
Comment at: clangd/ClangdUnit.cpp:714
-void CppFile::cancelRebuilds() {
+void CppFile::cancelRebuild() { deferCancelRebuild().get(); }
+
Somewhat orthogonal question - why CppFile? We do support other languages in
Clangd, right? (C / Obj-C)
I'd have expected this to be called ClangdUnit, especially given the file name.
Comment at: clangd/ClangdUnit.cpp:735
+
+ std::shared_ptr That = shared_from_this();
+ return std::async(std::launch::deferred, [That, RequestRebuildCounter]() {
Now I know why I don't like the shard_ptr, this is all pretty awkward.
Comment at: clangd/ClangdUnit.cpp:738
+std::unique_lock Lock(That->Mutex);
+CppFile *This = &*That;
+This->RebuildCond.wait(Lock, [This, RequestRebuildCounter]() {
Why do we need that instead of just calling the bound shared_ptr This?
https://reviews.llvm.org/D36397
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36397: [clangd] Fixed a data race.
ilya-biryukov added a comment. In https://reviews.llvm.org/D36397#833912, @klimek wrote: > Yea, we'll probably want to add a "smash it hard with parallel" requests kind > of test to catch these things. You're right, there is probably not a better > solution for this specific bug. Added a relevant test to https://reviews.llvm.org/D36261. It actually found this problem (after repeatedly running it for some time). https://reviews.llvm.org/D36397 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36397: [clangd] Fixed a data race.
klimek added a comment. In https://reviews.llvm.org/D36397#833890, @ilya-biryukov wrote: > In https://reviews.llvm.org/D36397#833883, @klimek wrote: > > > Tests? > > > TSAN does not catch this (as it's a logical error) and it requires a rather > bizarre timing of file reparses to trigger. > I couldn't come up with an example that would reproduce this. Yea, we'll probably want to add a "smash it hard with parallel" requests kind of test to catch these things. You're right, there is probably not a better solution for this specific bug. https://reviews.llvm.org/D36397 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36397: [clangd] Fixed a data race.
ilya-biryukov added a comment. In https://reviews.llvm.org/D36397#833883, @klimek wrote: > Tests? TSAN does not catch this (as it's a logical error) and it requires a rather bizarre timing of file reparses to trigger. I couldn't come up with an example that would reproduce this. https://reviews.llvm.org/D36397 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36397: [clangd] Fixed a data race.
klimek added a comment. Tests? https://reviews.llvm.org/D36397 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36397: [clangd] Fixed a data race.
ilya-biryukov created this revision.
Calling addDocument after removeDocument could have resulted in an
invalid program state (AST and Preamble for the valid document could
have been incorrectly removed).
This commit also includes an improved CppFile::cancelRebuild
implementation that allows to cancel reparse without waiting for
ongoing rebuild to finish.
https://reviews.llvm.org/D36397
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
Index: clangd/ClangdUnit.h
===
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -151,9 +151,17 @@
CppFile(CppFile const &) = delete;
CppFile(CppFile &&) = delete;
- /// Cancels all scheduled rebuilds and sets AST and Preamble to nulls.
+ /// Cancels a scheduled rebuild, if any, and sets AST and Preamble to nulls.
/// If a rebuild is in progress, will wait for it to finish.
- void cancelRebuilds();
+ void cancelRebuild();
+
+ /// Similar to deferRebuild, but sets both Preamble and AST to nulls instead
+ /// of doing an actual parsing. Returned future is a deferred computation that
+ /// will wait for any ongoing rebuilds to finish and actually set the AST and
+ /// Preamble to nulls. It can be run on a different thread.
+ /// This function is useful to cancel ongoing rebuilds, if any, before
+ /// removing CppFile.
+ std::future deferCancelRebuild();
/// Rebuild AST and Preamble synchronously on the calling thread.
/// Returns a list of diagnostics or a llvm::None, if another rebuild was
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -711,10 +711,12 @@
ASTFuture = ASTPromise.get_future();
}
-void CppFile::cancelRebuilds() {
+void CppFile::cancelRebuild() { deferCancelRebuild().get(); }
+
+std::future CppFile::deferCancelRebuild() {
std::unique_lock Lock(Mutex);
// Cancel an ongoing rebuild, if any, and wait for it to finish.
- ++this->RebuildCounter;
+ unsigned RequestRebuildCounter = ++this->RebuildCounter;
// Rebuild asserts that futures aren't ready if rebuild is cancelled.
// We want to keep this invariant.
if (futureIsReady(PreambleFuture)) {
@@ -725,12 +727,28 @@
ASTPromise = std::promise();
ASTFuture = ASTPromise.get_future();
}
- // Now wait for rebuild to finish.
- RebuildCond.wait(Lock, [this]() { return !this->RebuildInProgress; });
- // Return empty results for futures.
- PreamblePromise.set_value(nullptr);
- ASTPromise.set_value(std::make_shared(llvm::None));
+ Lock.unlock();
+ // Notify about changes to RebuildCounter.
+ RebuildCond.notify_all();
+
+ std::shared_ptr That = shared_from_this();
+ return std::async(std::launch::deferred, [That, RequestRebuildCounter]() {
+std::unique_lock Lock(That->Mutex);
+CppFile *This = &*That;
+This->RebuildCond.wait(Lock, [This, RequestRebuildCounter]() {
+ return !This->RebuildInProgress ||
+ This->RebuildCounter != RequestRebuildCounter;
+});
+
+// This computation got cancelled itself, do nothing.
+if (This->RebuildCounter != RequestRebuildCounter)
+ return;
+
+// Set empty results for Promises.
+That->PreamblePromise.set_value(nullptr);
+That->ASTPromise.set_value(std::make_shared(llvm::None));
+ });
}
llvm::Optional
@@ -767,6 +785,8 @@
this->ASTFuture = this->ASTPromise.get_future();
}
} // unlock Mutex.
+ // Notify about changes to RebuildCounter.
+ RebuildCond.notify_all();
// A helper to function to finish the rebuild. May be run on a different
// thread.
@@ -916,7 +936,10 @@
if (WasCancelledBeforeConstruction)
return;
- File.RebuildCond.wait(Lock, []() { return !File.RebuildInProgress; });
+ File.RebuildCond.wait(Lock, [, RequestRebuildCounter]() {
+return !File.RebuildInProgress ||
+ File.RebuildCounter != RequestRebuildCounter;
+ });
WasCancelledBeforeConstruction = File.RebuildCounter != RequestRebuildCounter;
if (WasCancelledBeforeConstruction)
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -233,6 +233,13 @@
std::string dumpAST(PathRef File);
private:
+ std::future
+ scheduleReparseAndDiags(PathRef File, VersionedDraft Contents,
+ std::shared_ptr Resources,
+ Tagged TaggedFS);
+
+ std::future scheduleCancelRebuild(std::shared_ptr Resources);
+
GlobalCompilationDatabase
DiagnosticsConsumer
FileSystemProvider
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -143,61 +143,14 @@
auto TaggedFS = FSProvider.getTaggedFileSystem(File);
std::shared_ptr Resources =
