Skip to content

[NFC] Avoid data race condition. #118664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

[NFC] Avoid data race condition. #118664

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35f7955
[NFC] Avoid data race condition.
zahiraam Dec 4, 2024
6dd0aa3
Remove unnecessary lock.
zahiraam Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang-tools-extra/clangd/TUScheduler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,7 @@ class PreambleThread {
}

{
std::unique_lock<std::mutex> Lock(Mutex);
WithContext Guard(std::move(CurrentReq->Ctx));
Comment on lines +504 to 505
Copy link
Contributor

@tahonermann tahonermann Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I don't think this solves the issue by itself. Just prior to exiting the above block, we are assured that CurrentReq has been assigned and thus holds an object. At this point, the lock on Mutex has been released which means it is possible that some other thread executing this same function might acquire the lock on Mutex and reset CurrentReq at line 517/518 below before this code is reached. The possibility still exists then that CurrentReq doesn't hold an object at this point (or holds a partially constructed/destructed object; note that std::optional doesn't promise atomic operations).

I'm not sure what the right fix is for this issue at this point. I think it might be to move *NextReq above into a local Request variable and change the CurrentReq data member to just be a bool that indicates whether a request is being processed. That would avoid any possibility of concurrent modification of the Request object while still preserving the ability to check if a request is being processed in blockUntilIdle().

Copy link
Contributor

@tahonermann tahonermann Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per private discussion, we determined that there isn't actually a bug here so long as a PreambleThread object is only ever scheduled to run on one thread. That appears to be the case; the only instance of a PreambleThread object is the PreamblePeer member of ASTWorker and ASTWorker::create() only spawns one task to run PreambleThread::run(). I think this PR can be abandoned.

Copy link
Contributor Author

@zahiraam zahiraam Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tahonermann. I had forgotten about it. Closing it.

// Note that we don't make use of the ContextProvider here.
// Preamble tasks are always scheduled by ASTWorker tasks, and we
Expand Down
Loading