[clangd] Mark AsyncTaskRunner::runAsync as const

[kadircet]

Summary

RunAsync performs a write that's internally synchronized, so in theory
it shouldn't introduce any data-races. This will enable spawning async tasks in
const contexts.

[sammccall]

AsyncTaskRunner is definitely threadsafe, but const doesn't exactly mean threadsafe...

This might be OK or maybe we should put mutable at the callsite - can you show the real example?

[kadircet]

In D90747#2379406, @sammccall wrote:

AsyncTaskRunner is definitely threadsafe, but const doesn't exactly mean threadsafe...

Right, I was also trying to punt on the idea of "writes are synchronized" rather than promising threadsafety here.

This might be OK or maybe we should put mutable at the callsite - can you show the real example?

Oops, sorry forgot to update the stack for this one. The usage is in https://reviews.llvm.org/D90750, line 86 of ProjectAware.cpp specifically.
We definitely can make the AsyncTaskRunner mutable there too, but I would feel bad about mutating a "mutable" member without holding a lock in general, hence this change.

[sammccall]

This isn't a hill I'm going to die on, it doesn't matter a great deal - so if this solves
But I don't think this is an appropriate use of const - it changes the object in a significant way.

"significant" is vague, but i think "wait()ing on a threadpool will now block" qualifies whereas "destroying (long-lived) ProjectAwareIndex will now block" doesn't.
It's as much about the scope of the object as the nature of the change.

A couple of other examples of "thread pool" classes don't make the "schedule" option const:

• http://threadpool.sourceforge.net/reference/a00024.html
• https://pocoproject.org/docs/Poco.ThreadPool.html

(Boost's threadpool is threadsafe like this one)

In D90747#2383716, @kadircet wrote:

This might be OK or maybe we should put mutable at the callsite - can you show the real example?

Oops, sorry forgot to update the stack for this one. The usage is in https://reviews.llvm.org/D90750, line 86 of ProjectAware.cpp specifically.

Yeah, this does seem to me like a place to mark the class as "threadsafe" and the member as mutable.

We definitely can make the AsyncTaskRunner mutable there too, but I would feel bad about mutating a "mutable" member without holding a lock in general, hence this change.

I don't think that's bad - mutating requires access to be synchronized, either externally (holding a lock) or internally (class is threadsafe).
This is why it's safe to mutate (lock) the mutable mutex without holding yet another mutex first: the mutex class is threadsafe.

[kadircet]

SG, gonna abandon this one and mark the threadpool as mutable in the ProjectAwareIndex instead.

clang-tools-extra/clangd/support/Threading.h

 class AsyncTaskRunner {
 public:
   /// Destructor waits for all pending tasks to finish.
   ~AsyncTaskRunner();
 
   void wait() const { (void)wait(Deadline::infinity()); }
   LLVM_NODISCARD bool wait(Deadline D) const;
   // The name is used for tracing and debugging (e.g. to name a spawned thread).
-  void runAsync(const llvm::Twine &Name, llvm::unique_function<void()> Action);
+  void runAsync(const llvm::Twine &Name,
+                llvm::unique_function<void()> Action) const;
 
 private:
   mutable std::mutex Mutex;
   mutable std::condition_variable TasksReachedZero;
-  std::size_t InFlightTasks = 0;
+  mutable std::size_t InFlightTasks = 0;
 };
 
 /// Runs \p Action asynchronously with a new std::thread. The context will be
 /// propagated.
 template <typename T>
 std::future<T> runAsync(llvm::unique_function<T()> Action) {
   return std::async(
       std::launch::async,

clang-tools-extra/clangd/support/Threading.cpp

 
 bool AsyncTaskRunner::wait(Deadline D) const {
   std::unique_lock<std::mutex> Lock(Mutex);
   return clangd::wait(Lock, TasksReachedZero, D,
                       [&] { return InFlightTasks == 0; });
 }
 
 void AsyncTaskRunner::runAsync(const llvm::Twine &Name,
-                               llvm::unique_function<void()> Action) {
+                               llvm::unique_function<void()> Action) const {
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     ++InFlightTasks;
   }
 
   auto CleanupTask = llvm::make_scope_exit([this]() {
     std::lock_guard<std::mutex> Lock(Mutex);
     int NewTasksCnt = --InFlightTasks;