[PATCH] D65387: [clangd] Add a callback mechanism for handling responses from client.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this looks a lot better. There are a bunch of details that can be simplified and names that could be clearer, but I think this is the right shape.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:161
+
+    Callback<llvm::json::Value> FoundCB = nullptr;
+    if (auto IntID = ID.getAsInteger()) {
----------------
nit: maybe call this ReplyHandler?


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:165
+      // Find a corresponding callback for the request ID;
+      // We can't use binary search here as ReplyCallbacks is not guaranteed to
+      // be sorted in the multi-thread environment.
----------------
this got me thinking - we're using an `atomic<int>` to assign sequential IDs, but then locking to put them into a map. The reward for using two types of concurrency primitives is our array is almost-but-not-quite sorted.

I think we should demote `NextCallID` to a regular int, and guard it with the same mutex as ReplyCallbacks. (Just call it `CallMutex`?) Then bindReply can lock the mutex, pick a call id, add the callback to the map, and return the ID.

I still don't think we need to binary search though. The list is 100 at most and ~1 in practice.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:167
+      // be sorted in the multi-thread environment.
+      auto Found =
+          llvm::find_if(ReplyCallbacks, [&IntID](const ReplyCallback &RCB) {
----------------
nit: a normal loop with if/break seems clearer than find_if here


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:177
+
+    if (FoundCB) {
+      // Found a corresponding callback from the queue.
----------------
Might just be me, but I think I'd find it slightly to avoid the LogAndRun lambda by overwriting the callback:
```
// If there was no callback in the table, the client is sending us an unsolicited reply!
if (FoundCB == nullptr)
  Found = [&ID](...) { elog and consume error; }

// inline LogAndRun here
```


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:184
+    // No callback being found, run a default log callback.
+    LogAndRun([&ID](decltype(Result) Result) {
+      elog("received a reply with ID {0}, but there was no such call", ID);
----------------
nit: we tend to use `decltype(Result)` to specify the type of `Result` when we're pseudo-capturing it in a lambda using `Bind()`. That's not the case here, so please spell out the type.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:218
+      // take out and run the oldest pending callback if we overflow.
+      if (ReplyCallbacks.size() > MaxCallbacks) {
+        OldestCB = std::move(ReplyCallbacks.front());
----------------
elog("more than {0} outstanding LSP calls, forgetting about {1}")


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:224
+    if (OldestCB)
+      OldestCB->CB(
+          llvm::formatv("failed to receive a client reply for request ({0})",
----------------
you're returning a string to the callback, I think you meant to return an error?


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:278
     ~ReplyOnce() {
-      if (Server && !Replied) {
+      if (Server && !Server->isDestructing() && !Replied) {
         elog("No reply to message {0}({1})", Method, ID);
----------------
Need a comment about the server being destroyed/unreplied calls case.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:315
+  //
+  // We bound the maximum size to the pending queue to prevent memory leakage
+  // for cases where LSP clients don't reply for the request.
----------------
nit: physically this is a deque, but it functions more like a map and I think it might be easier to refer to that way.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:318
+  //
+  // If the queue overflows, we assume that the client didn't reply the
+  // oldest request, and run the corresponding callback which replies an Error
----------------
nit: I think this part of the comment belongs in the implementation rather than on the variable


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:325
+  // a corresponding request.
+  struct ReplyCallback {
+    int RequestID;
----------------
hmm, maybe just use `pair<int, Callback<Value>>` here? That way it looks even more like a map


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:580
   };
+  auto ReplyApplyEdit =
+      [](decltype(Reply) Reply,
----------------
I can't parse this name.

Looking at the code... it's doing something like `ReplyAfterApplyingEdits`

It should take the success message as a parameter, I think - at the moment you're replying "fix applied" even in the tweak case


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:580
   };
+  auto ReplyApplyEdit =
+      [](decltype(Reply) Reply,
----------------
sammccall wrote:
> I can't parse this name.
> 
> Looking at the code... it's doing something like `ReplyAfterApplyingEdits`
> 
> It should take the success message as a parameter, I think - at the moment you're replying "fix applied" even in the tweak case
So this method is already a tangled mess, and this patch makes it worse. However, it's *conceptually* exactly the right API, just callbacks and Expected and LSP conspire to be awkward here.
We should refactor this, but this patch isn't the right time. Can you add a `//FIXME: this method is tangled and confusing, refactor it` or so?


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:591
+              llvm::inconvertibleErrorCode(),
+              ("edits fail to applied: " + Reason).c_str()));
+        }
----------------
nit: edits were not applied (or edits failed to apply)


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:54
 
+  bool isDestructing() const { return Destructing; }
+
----------------
MessageHandler is a nested class and so has access to Destructing already, no need for this function


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:138
 
+  std::atomic<bool> Destructing = {false};
+
----------------
nit: Destroying
but I think `IsBeingDestroyed` would be clearer, as destroy is transitive

this needs a comment


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:159
+    // Wrap the callback with LSP conversion and error-handling.
+    auto Reply = [](decltype(CB) CB,
+                    llvm::Expected<llvm::json::Value> RawResponse) {
----------------
nit: HandleReply?
lambdas often have names like functions, so this reads like it's a function that replies


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:174
+  }
+  void callImpl(StringRef Method, llvm::json::Value Params,
+                Callback<llvm::json::Value> CB);
----------------
nit: I know I suggested this name but I think `callRaw` would be better


================
Comment at: clang-tools-extra/clangd/Protocol.h:877
+struct ApplyWorkspaceEditResponse {
+  bool applied;
+  llvm::Optional<std::string> failureReason;
----------------
needs a default value

`= true`, I guess


================
Comment at: clang-tools-extra/clangd/test/request-reply.test:5
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"auto i = 0;"}}}
+---
+---
----------------
nit, extra line


================
Comment at: clang-tools-extra/clangd/test/request-reply.test:22
+---
+{"jsonrpc":"2.0","id":0,"result":{"applied":false}}
+#      CHECK:  "code": -32001,
----------------
please use increasing IDs and don't reuse them. (In general, but particularly in this test)


================
Comment at: clang-tools-extra/clangd/test/request-reply.test:23
+{"jsonrpc":"2.0","id":0,"result":{"applied":false}}
+#      CHECK:  "code": -32001,
+# CHECK-NEXT:  "message": "edits fail to applied:
----------------
can you verify that the error comes with the right ID? (and similar below)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65387/new/

https://reviews.llvm.org/D65387