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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:150
+    // Log the reply.
+    log("<-- reply({0})", ID);
+    Server.onReply(std::move(ID), std::move(Result));
----------------
We want to keep logging errors (not just at verbose level).

If you need a non-destructive way to get the error message for logging, I think it's unfortunately:
```
if (Result) {
  log();
  handle(move(Result));
} else {
  err = Result.takeError();
  log(err);
  handle(move(err));
}
```

Note I think our logging function has support for printing both rvalue errors (consuming them) and lvalue errors (not consuming them). But failing that, operator<< doesn't consume the error.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:314
   auto ID = NextCallID++;
+  auto StrID = llvm::to_string(ID);
+  {
----------------
we know the underlying ID is an integer (because we generate it above), so why not just key on the integer directly?
In Reply, if the ID isn't an integer then we know there's no callback.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:317
+    std::lock_guard<std::mutex> Lock(ReplyCallbacksMutex);
+    ReplyCallbacks[StrID] = std::move(CB);
+  }
----------------
jkorous wrote:
> Should we care if there's already another callback registered for this `StrID`?
I think we should assert on this. NextCallID should never overflow.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:317
+    std::lock_guard<std::mutex> Lock(ReplyCallbacksMutex);
+    ReplyCallbacks[StrID] = std::move(CB);
+  }
----------------
sammccall wrote:
> jkorous wrote:
> > Should we care if there's already another callback registered for this `StrID`?
> I think we should assert on this. NextCallID should never overflow.
as Jan points out, this is an unbounded leak if the client never replies to requests.

the most obvious approach to fix this is to bound the size and drop e.g. oldest pending query if we overflow. (we can exploit that IDs are sequential integers, if that's useful)

I mention this because if the size is bounded to say 100 and small in practice, you might end up wanting to use deque or (linked) list


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:336
+    if (Result)
+      log("no corresponding callback for the reply ({0})", ID);
+    else
----------------
elog, this is a protocol error

The message is too low level, it describes the implementation rather than the protocol. Prefer e.g. `received a reply with ID {0}, but there was no such call`

No need to handle error vs non-error here, it should be logged generally above (whether there was a missing callback or not)


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:526
     Edit.edit = std::move(WE);
     // Ideally, we would wait for the response and if there is no error, we
     // would reply success/failure to the original RPC.
----------------
While I like keeping the scope small, it's not really possible to evaluate this change without using it at least once.

Can you use the new facility to address this comment?
This would look something like:
 - ApplyEdit takes a Callback<ApplyWorkspaceEditResponse>
 - callsites look like this
```
ApplyEdit(*Params.workspaceEdit, bind(std::move(Reply), [](decltype(Reply) Reply, Expected<ApplyWorkspaceEditResponse> Response) {
  if (!Response)
    return Reply(Response.takeError());
  if (!Response->applied)
    return Reply(makeStringError("edits not applied: " + Response->failureReason));
  return Reply("Fix applied");
});
```

(This is pretty verbose, but seems realistic for what we're actually going to do).

This function is a bit of a complicated monster, so feel free to just handle apply fix, or (better) to factor the code differently so it's reasonable to handle both.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:152
+  // request.
+  using ReplyCallback = std::function<void(llvm::Expected<llvm::json::Value>)>;
+  std::mutex ReplyCallbacksMutex;
----------------
this type is just `clangd::Callback<llvm::json::Value>()`. (modulo supporting move-only captures, which we want I think)
I'm not sure that's worth abbreviating.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:155
+  // Request ID => reply Callbacks
+  llvm::StringMap<ReplyCallback> ReplyCallbacks;
+
----------------
the corresponding state for request cancellation lives in MessageHandler, I think this one should too.

Similarly, onReply fits better in MessageHandler than here. `call()` belongs here though, so I guess `call()` needs to call something like `MessageHandler::bindReply(ID, Callback<T>)` or so


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:157
+
+  void call(StringRef Method, llvm::json::Value Params, ReplyCallback CB);
+  void onReply(llvm::json::Value ID, llvm::Expected<llvm::json::Value> Result);
----------------
This API makes the caller of `call()` responsible for dealing with converting the arbitrary JSON returned by the client into the protocol object, and handling errors (which they'll do inconsistently).

I'd suggest instead a signature like
```
template <typename Rsp=json::Value>
void call(StringRef Method, json::Value Params, Callback<Rsp> CB=nullptr)
```

where the template body would wrap CB with conversion and error-handling logic, and then add the "generic" Callback<json::Value> to the map.

you can see bind() - this is similar to how incoming messages are handled.

Making this a template directly forces it to be in the header: to mitigate this you can delegating to a non-template `callImpl()` as soon as you've erased the type to Callback<llvm::Value>, and/or putting the type-specific call() in MessageHandler (which seems pretty weird to me, so maybe not?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65387