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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
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);
----------------
sammccall wrote:
> Need a comment about the server being destroyed/unreplied calls case.
You've added a comment that repeats what the code does, please don't do that :-)

Instead, explain the context around this case. e.g.
"There's one legitimate reason to never reply to a request: clangd's request handler sent a call to the client (e.g. applyWorkspaceEdit) and the client never replied. In this case, the ReplyOnce is owned by ClangdServer's reply callback table and is destroyed along with the server. We don't attempt to send a reply in this case, there's little to be gained from doing so."


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:205
+  // Return a call id of the request.
+  int bindReply(Callback<llvm::json::Value> Reply) {
+    llvm::Optional<std::pair<int, Callback<llvm::json::Value>>> OldestCB;
----------------
nit: I think this function could return json::Value, encapsulating the fact that we use integers inside the MessageHandler class.

(Or does something outside the class depend on this?)


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:228
+                        OldestCB->first)));
+    assert(ID != -1 && "ID must be set");
+    return ID;
----------------
There's no complicated flow control around this - I'd suggest instead leaving ID uninitialized and letting msan flag this if it goes wrong.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:317
   llvm::StringMap<std::function<void(llvm::json::Value, ReplyOnce)>> Calls;
+  // The maximum number of callbacks hold in clangd.
+  //
----------------
"held" or "to hold"


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:321
+  // for cases where LSP clients don't reply for the request.
+  static constexpr int MaxCallbacks = 100;
+  mutable std::mutex CallMutex;
----------------
more specific name: MaxReplyCallbacks or even better MaxOutstandingCalls?


================
Comment at: clang-tools-extra/clangd/test/fixits-command.test:206
 ---
+{"jsonrpc":"2.0","id":0,"result":{"applied":true}}
+#      CHECK:  "id": 4,
----------------
this ID is reused


================
Comment at: clang-tools-extra/clangd/test/request-reply.test:28
+---
+{"jsonrpc":"2.0","id":4,"method":"workspace/executeCommand","params":{"command":"clangd.applyTweak","arguments":[{"file":"file:///clangd-test/main.cpp","selection":{"end":{"character":4,"line":0},"start":{"character":0,"line":0}},"tweakID":"ExpandAutoType"}]}}
+#      CHECK:  "id": 1,
----------------
ID reuse here (4 was previously used on line 6)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65387