Skip to content

Commit d7eada4

Browse files
committed
[SourceKit] Resolve a nondeterministic deadlock in SourceKit while cancelling
This fixes a race-conditioned deadlock which could occur while cancelling SourceKit AST build request We have one thread that claimed `CancellationRequestCallbackMtx` in `SwiftASTConsumer::requestCancellation` and wants to claim `ConsumersAndResultMtx` in `ASTBuildOperation::requestConsumerCancellation` Another thread claimed `ConsumersAndResultMtx` in `ASTBuildOperation::schedule` and now wants to claim `CancellationRequestCallbackMtx` in `SwiftASTConsumer::removeCancellationRequestCallback`. In both cases we could actually release one lock before claiming the other. Fixes rdar://90870793
1 parent 1e1d4e3 commit d7eada4

File tree

1 file changed

+21
-17
lines changed

1 file changed

+21
-17
lines changed

tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1137,38 +1137,42 @@ void ASTBuildOperation::schedule(WorkQueue Queue) {
11371137
std::string Error;
11381138
assert(!Result && "We should only be producing a result once");
11391139
ASTUnitRef AST = buildASTUnit(Error);
1140+
SmallVector<SwiftASTConsumerRef, 4> LocalConsumers;
11401141
{
11411142
llvm::sys::ScopedLock L(ConsumersAndResultMtx);
11421143
bool WasCancelled = CancellationFlag->load(std::memory_order_relaxed);
11431144
Result.emplace(AST, Error, WasCancelled);
1144-
for (auto &Consumer : Consumers) {
1145-
informConsumer(Consumer);
1146-
}
1145+
LocalConsumers = Consumers;
11471146
Consumers = {};
11481147
}
1148+
for (auto &Consumer : LocalConsumers) {
1149+
informConsumer(Consumer);
1150+
}
11491151
DidFinishCallback();
11501152
},
11511153
/*isStackDeep=*/true);
11521154
}
11531155

11541156
bool ASTBuildOperation::addConsumer(SwiftASTConsumerRef Consumer) {
1155-
llvm::sys::ScopedLock L(ConsumersAndResultMtx);
1156-
if (isCancelled()) {
1157-
return false;
1158-
}
1159-
if (Result) {
1160-
informConsumer(Consumer);
1161-
} else {
1157+
{
1158+
llvm::sys::ScopedLock L(ConsumersAndResultMtx);
1159+
if (isCancelled()) {
1160+
return false;
1161+
}
1162+
if (Result) {
1163+
informConsumer(Consumer);
1164+
return true;
1165+
}
11621166
assert(OperationState != State::Finished);
1163-
auto WeakThis = std::weak_ptr<ASTBuildOperation>(shared_from_this());
11641167
Consumers.push_back(Consumer);
1165-
Consumer->setCancellationRequestCallback(
1166-
[WeakThis](SwiftASTConsumerRef Consumer) {
1167-
if (auto This = WeakThis.lock()) {
1168-
This->requestConsumerCancellation(Consumer);
1169-
}
1170-
});
11711168
}
1169+
auto WeakThis = std::weak_ptr<ASTBuildOperation>(shared_from_this());
1170+
Consumer->setCancellationRequestCallback(
1171+
[WeakThis](SwiftASTConsumerRef Consumer) {
1172+
if (auto This = WeakThis.lock()) {
1173+
This->requestConsumerCancellation(Consumer);
1174+
}
1175+
});
11721176
return true;
11731177
}
11741178

0 commit comments

Comments
 (0)