[PATCH] D53399: [clangd] Ensure that we reply to each call exactly once. NFC (I think!)
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE345144: [clangd] Ensure that we reply to each call exactly
once. NFC (I think!) (authored by sammccall, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D53399?vs=170866&id=170881#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53399
Files:
clangd/ClangdLSPServer.cpp
clangd/Trace.h
Index: clangd/Trace.h
===
--- clangd/Trace.h
+++ clangd/Trace.h
@@ -87,6 +87,7 @@
/// Mutable metadata, if this span is interested.
/// Prefer to use SPAN_ATTACH rather than accessing this directly.
+ /// The lifetime of Args is the whole event, even if the Span dies.
llvm::json::Object *const Args;
private:
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -105,16 +105,21 @@
}
bool onCall(StringRef Method, json::Value Params, json::Value ID) override {
+// Calls can be canceled by the client. Add cancellation context.
+WithContext WithCancel(cancelableRequestContext(ID));
+trace::Span Tracer(Method);
+SPAN_ATTACH(Tracer, "Params", Params);
+ReplyOnce Reply(ID, Method, &Server, Tracer.Args);
log("<-- {0}({1})", Method, ID);
if (!Server.Server && Method != "initialize") {
elog("Call {0} before initialization.", Method);
- Server.reply(ID, make_error("server not initialized",
-ErrorCode::ServerNotInitialized));
+ Reply(make_error("server not initialized",
+ ErrorCode::ServerNotInitialized));
} else if (auto Handler = Calls.lookup(Method))
- Handler(std::move(Params), std::move(ID));
+ Handler(std::move(Params), std::move(Reply));
else
- Server.reply(ID, make_error("method not found",
-ErrorCode::MethodNotFound));
+ Reply(
+ make_error("method not found", ErrorCode::MethodNotFound));
return true;
}
@@ -128,36 +133,19 @@
}
// Bind an LSP method name to a call.
- template
+ template
void bind(const char *Method,
-void (ClangdLSPServer::*Handler)(const Param &, Callback)) {
+void (ClangdLSPServer::*Handler)(const Param &, Callback)) {
Calls[Method] = [Method, Handler, this](json::Value RawParams,
-json::Value ID) {
+ReplyOnce Reply) {
Param P;
- if (!fromJSON(RawParams, P)) {
+ if (fromJSON(RawParams, P)) {
+(Server.*Handler)(P, std::move(Reply));
+ } else {
elog("Failed to decode {0} request.", Method);
-Server.reply(ID, make_error("failed to decode request",
- ErrorCode::InvalidRequest));
-return;
+Reply(make_error("failed to decode request",
+ ErrorCode::InvalidRequest));
}
- trace::Span Tracer(Method);
- SPAN_ATTACH(Tracer, "Params", RawParams);
- auto *Trace = Tracer.Args; // We attach reply from another thread.
- // Calls can be canceled by the client. Add cancellation context.
- WithContext WithCancel(cancelableRequestContext(ID));
- // FIXME: this function should assert it's called exactly once.
- (Server.*Handler)(P, [this, ID, Trace](Expected Result) {
-if (Result) {
- if (Trace)
-(*Trace)["Reply"] = *Result;
- Server.reply(ID, json::Value(std::move(*Result)));
-} else {
- auto Err = Result.takeError();
- if (Trace)
-(*Trace)["Error"] = to_string(Err);
- Server.reply(ID, std::move(Err));
-}
- });
};
}
@@ -178,8 +166,65 @@
}
private:
+ // Function object to reply to an LSP call.
+ // Each instance must be called exactly once, otherwise:
+ // - the bug is logged, and (in debug mode) an assert will fire
+ // - if there was no reply, an error reply is sent
+ // - if there were multiple replies, only the first is sent
+ class ReplyOnce {
+std::atomic Replied = {false};
+json::Value ID;
+std::string Method;
+ClangdLSPServer *Server; // Null when moved-from.
+json::Object *TraceArgs;
+
+ public:
+ReplyOnce(const json::Value &ID, StringRef Method, ClangdLSPServer *Server,
+ json::Object *TraceArgs)
+: ID(ID), Method(Method), Server(Server), TraceArgs(TraceArgs) {
+ assert(Server);
+}
+ReplyOnce(ReplyOnce &&Other)
+: Replied(Other.Replied.load()), ID(std::move(Other.ID)),
+ Method(std::move(Other.Method)), Server(Other.Server),
+ TraceArgs(Other.TraceArgs) {
+ Other.Server = nullptr;
+}
+ReplyOnce& operator=(ReplyOnce&&) = delete;
+ReplyOnce(const Rep
[PATCH] D53399: [clangd] Ensure that we reply to each call exactly once. NFC (I think!)
sammccall added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:112
+SPAN_ATTACH(Tracer, "Params", Params);
+ReplyOnce Reply(ID, Method, &Server, Tracer.Args);
log("<-- {0}({1})", Method, ID);
ioeric wrote:
> Do we have guarantee that `Tracer.Args` outlives `Reply`?
Yes, assuming the Handler propagates contexts properly.
The args are owned by the context created by the `trace::Span`.
Added a comment to `Span` to guarantee this.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53399
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53399: [clangd] Ensure that we reply to each call exactly once. NFC (I think!)
sammccall updated this revision to Diff 170866.
sammccall added a comment.
Clarify Span::Args lifetime.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53399
Files:
clangd/ClangdLSPServer.cpp
clangd/Trace.h
Index: clangd/Trace.h
===
--- clangd/Trace.h
+++ clangd/Trace.h
@@ -87,6 +87,7 @@
/// Mutable metadata, if this span is interested.
/// Prefer to use SPAN_ATTACH rather than accessing this directly.
+ /// The lifetime of Args is the whole event, even if the Span dies.
llvm::json::Object *const Args;
private:
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -105,16 +105,21 @@
}
bool onCall(StringRef Method, json::Value Params, json::Value ID) override {
+// Calls can be canceled by the client. Add cancellation context.
+WithContext WithCancel(cancelableRequestContext(ID));
+trace::Span Tracer(Method);
+SPAN_ATTACH(Tracer, "Params", Params);
+ReplyOnce Reply(ID, Method, &Server, Tracer.Args);
log("<-- {0}({1})", Method, ID);
if (!Server.Server && Method != "initialize") {
elog("Call {0} before initialization.", Method);
- Server.reply(ID, make_error("server not initialized",
-ErrorCode::ServerNotInitialized));
+ Reply(make_error("server not initialized",
+ ErrorCode::ServerNotInitialized));
} else if (auto Handler = Calls.lookup(Method))
- Handler(std::move(Params), std::move(ID));
+ Handler(std::move(Params), std::move(Reply));
else
- Server.reply(ID, make_error("method not found",
-ErrorCode::MethodNotFound));
+ Reply(
+ make_error("method not found", ErrorCode::MethodNotFound));
return true;
}
@@ -128,36 +133,19 @@
}
// Bind an LSP method name to a call.
- template
+ template
void bind(const char *Method,
-void (ClangdLSPServer::*Handler)(const Param &, Callback)) {
+void (ClangdLSPServer::*Handler)(const Param &, Callback)) {
Calls[Method] = [Method, Handler, this](json::Value RawParams,
-json::Value ID) {
+ReplyOnce Reply) {
Param P;
- if (!fromJSON(RawParams, P)) {
+ if (fromJSON(RawParams, P)) {
+(Server.*Handler)(P, std::move(Reply));
+ } else {
elog("Failed to decode {0} request.", Method);
-Server.reply(ID, make_error("failed to decode request",
- ErrorCode::InvalidRequest));
-return;
+Reply(make_error("failed to decode request",
+ ErrorCode::InvalidRequest));
}
- trace::Span Tracer(Method);
- SPAN_ATTACH(Tracer, "Params", RawParams);
- auto *Trace = Tracer.Args; // We attach reply from another thread.
- // Calls can be canceled by the client. Add cancellation context.
- WithContext WithCancel(cancelableRequestContext(ID));
- // FIXME: this function should assert it's called exactly once.
- (Server.*Handler)(P, [this, ID, Trace](Expected Result) {
-if (Result) {
- if (Trace)
-(*Trace)["Reply"] = *Result;
- Server.reply(ID, json::Value(std::move(*Result)));
-} else {
- auto Err = Result.takeError();
- if (Trace)
-(*Trace)["Error"] = to_string(Err);
- Server.reply(ID, std::move(Err));
-}
- });
};
}
@@ -178,8 +166,65 @@
}
private:
+ // Function object to reply to an LSP call.
+ // Each instance must be called exactly once, otherwise:
+ // - the bug is logged, and (in debug mode) an assert will fire
+ // - if there was no reply, an error reply is sent
+ // - if there were multiple replies, only the first is sent
+ class ReplyOnce {
+std::atomic Replied = {false};
+json::Value ID;
+std::string Method;
+ClangdLSPServer *Server; // Null when moved-from.
+json::Object *TraceArgs;
+
+ public:
+ReplyOnce(const json::Value &ID, StringRef Method, ClangdLSPServer *Server,
+ json::Object *TraceArgs)
+: ID(ID), Method(Method), Server(Server), TraceArgs(TraceArgs) {
+ assert(Server);
+}
+ReplyOnce(ReplyOnce &&Other)
+: Replied(Other.Replied.load()), ID(std::move(Other.ID)),
+ Method(std::move(Other.Method)), Server(Other.Server),
+ TraceArgs(Other.TraceArgs) {
+ Other.Server = nullptr;
+}
+ReplyOnce& operator=(ReplyOnce&&) = delete;
+ReplyOnce(const ReplyOnce &) = delete;
+ReplyOnce& operator=(const ReplyOnce&) = delete;
+
+~ReplyOnce() {
+ if (Server && !Replied) {
+elog("No reply to message {0}({1})", Method, ID);
+
[PATCH] D53399: [clangd] Ensure that we reply to each call exactly once. NFC (I think!)
ioeric added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:112
+SPAN_ATTACH(Tracer, "Params", Params);
+ReplyOnce Reply(ID, Method, &Server, Tracer.Args);
log("<-- {0}({1})", Method, ID);
Do we have guarantee that `Tracer.Args` outlives `Reply`?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53399
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53399: [clangd] Ensure that we reply to each call exactly once. NFC (I think!)
sammccall updated this revision to Diff 170593.
sammccall added a comment.
Make ReplyOnce move-only.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53399
Files:
clangd/ClangdLSPServer.cpp
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -105,16 +105,21 @@
}
bool onCall(StringRef Method, json::Value Params, json::Value ID) override {
+// Calls can be canceled by the client. Add cancellation context.
+WithContext WithCancel(cancelableRequestContext(ID));
+trace::Span Tracer(Method);
+SPAN_ATTACH(Tracer, "Params", Params);
+ReplyOnce Reply(ID, Method, &Server, Tracer.Args);
log("<-- {0}({1})", Method, ID);
if (!Server.Server && Method != "initialize") {
elog("Call {0} before initialization.", Method);
- Server.reply(ID, make_error("server not initialized",
-ErrorCode::ServerNotInitialized));
+ Reply(make_error("server not initialized",
+ ErrorCode::ServerNotInitialized));
} else if (auto Handler = Calls.lookup(Method))
- Handler(std::move(Params), std::move(ID));
+ Handler(std::move(Params), std::move(Reply));
else
- Server.reply(ID, make_error("method not found",
-ErrorCode::MethodNotFound));
+ Reply(
+ make_error("method not found", ErrorCode::MethodNotFound));
return true;
}
@@ -128,36 +133,19 @@
}
// Bind an LSP method name to a call.
- template
+ template
void bind(const char *Method,
-void (ClangdLSPServer::*Handler)(const Param &, Callback)) {
+void (ClangdLSPServer::*Handler)(const Param &, Callback)) {
Calls[Method] = [Method, Handler, this](json::Value RawParams,
-json::Value ID) {
+ReplyOnce Reply) {
Param P;
- if (!fromJSON(RawParams, P)) {
+ if (fromJSON(RawParams, P)) {
+(Server.*Handler)(P, std::move(Reply));
+ } else {
elog("Failed to decode {0} request.", Method);
-Server.reply(ID, make_error("failed to decode request",
- ErrorCode::InvalidRequest));
-return;
+Reply(make_error("failed to decode request",
+ ErrorCode::InvalidRequest));
}
- trace::Span Tracer(Method);
- SPAN_ATTACH(Tracer, "Params", RawParams);
- auto *Trace = Tracer.Args; // We attach reply from another thread.
- // Calls can be canceled by the client. Add cancellation context.
- WithContext WithCancel(cancelableRequestContext(ID));
- // FIXME: this function should assert it's called exactly once.
- (Server.*Handler)(P, [this, ID, Trace](Expected Result) {
-if (Result) {
- if (Trace)
-(*Trace)["Reply"] = *Result;
- Server.reply(ID, json::Value(std::move(*Result)));
-} else {
- auto Err = Result.takeError();
- if (Trace)
-(*Trace)["Error"] = to_string(Err);
- Server.reply(ID, std::move(Err));
-}
- });
};
}
@@ -178,8 +166,65 @@
}
private:
+ // Function object to reply to an LSP call.
+ // Each instance must be called exactly once, otherwise:
+ // - the bug is logged, and (in debug mode) an assert will fire
+ // - if there was no reply, an error reply is sent
+ // - if there were multiple replies, only the first is sent
+ class ReplyOnce {
+std::atomic Replied = {false};
+json::Value ID;
+std::string Method;
+ClangdLSPServer *Server; // Null when moved-from.
+json::Object *TraceArgs;
+
+ public:
+ReplyOnce(const json::Value &ID, StringRef Method, ClangdLSPServer *Server,
+ json::Object *TraceArgs)
+: ID(ID), Method(Method), Server(Server), TraceArgs(TraceArgs) {
+ assert(Server);
+}
+ReplyOnce(ReplyOnce &&Other)
+: Replied(Other.Replied.load()), ID(std::move(Other.ID)),
+ Method(std::move(Other.Method)), Server(Other.Server),
+ TraceArgs(Other.TraceArgs) {
+ Other.Server = nullptr;
+}
+ReplyOnce& operator=(ReplyOnce&&) = delete;
+ReplyOnce(const ReplyOnce &) = delete;
+ReplyOnce& operator=(const ReplyOnce&) = delete;
+
+~ReplyOnce() {
+ if (Server && !Replied) {
+elog("No reply to message {0}({1})", Method, ID);
+assert(false && "must reply to all calls!");
+(*this)(make_error("server failed to reply",
+ ErrorCode::InternalError));
+ }
+}
+
+void operator()(Expected Reply) {
+ assert(Server && "moved-from!");
+ if (Replied.exchange(true)) {
+elog("Replied twice to message {0}({1})", Method, ID);
+assert(false && "must reply to eac
[PATCH] D53399: [clangd] Ensure that we reply to each call exactly once. NFC (I think!)
ioeric added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:182
+ // - if there were multiple replies, only the first is sent
+ class ReplyOnce {
+struct State {
As discussed offline, we could potentially make this non-copyable to make
harder to be duplicated and called more than once. And we could also get rid of
the shared `State` in the functor.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53399
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53399: [clangd] Ensure that we reply to each call exactly once. NFC (I think!)
sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, jfb, arphaman, jkorous,
MaskRay, ilya-biryukov.
In debug builds, getting this wrong will trigger asserts.
In production builds, it will send an error reply if none was sent,
and drop redundant replies. (And log).
No tests because this is always a programming error.
(We did have some cases of this, but I fixed them with the new dispatcher).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53399
Files:
clangd/ClangdLSPServer.cpp
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -106,11 +106,14 @@
bool onCall(StringRef Method, json::Value Params, json::Value ID) override {
log("<-- {0}({1})", Method, ID);
+// Calls can be canceled by the client. Add cancellation context.
+WithContext WithCancel(cancelableRequestContext(ID));
+ReplyOnce Reply(ID, Method, &Server);
if (auto Handler = Calls.lookup(Method))
- Handler(std::move(Params), std::move(ID));
+ Handler(std::move(Params), std::move(Reply));
else
- Server.reply(ID, llvm::make_error("method not found",
- ErrorCode::MethodNotFound));
+ Reply(llvm::make_error("method not found",
+ ErrorCode::MethodNotFound));
return true;
}
@@ -124,34 +127,31 @@
}
// Bind an LSP method name to a call.
- template
+ template
void bind(const char *Method,
-void (ClangdLSPServer::*Handler)(const Param &, Callback)) {
+void (ClangdLSPServer::*Handler)(const Param &, Callback)) {
Calls[Method] = [Method, Handler, this](json::Value RawParams,
-json::Value ID) {
+ReplyOnce Reply) {
Param P;
if (!fromJSON(RawParams, P)) {
elog("Failed to decode {0} request.", Method);
-Server.reply(ID, make_error("failed to decode request",
- ErrorCode::InvalidRequest));
+Reply(make_error("failed to decode request",
+ ErrorCode::InvalidRequest));
return;
}
trace::Span Tracer(Method);
SPAN_ATTACH(Tracer, "Params", RawParams);
auto *Trace = Tracer.Args; // We attach reply from another thread.
- // Calls can be canceled by the client. Add cancellation context.
- WithContext WithCancel(cancelableRequestContext(ID));
- // FIXME: this function should assert it's called exactly once.
- (Server.*Handler)(P, [this, ID, Trace](llvm::Expected Result) {
-if (Result) {
+ (Server.*Handler)(P, [Trace, Reply](llvm::Expected Res) {
+if (Res) {
if (Trace)
-(*Trace)["Reply"] = *Result;
- Server.reply(ID, json::Value(std::move(*Result)));
+(*Trace)["Reply"] = *Res;
+ Reply(json::Value(std::move(*Res)));
} else {
- auto Err = Result.takeError();
+ auto Err = Res.takeError();
if (Trace)
(*Trace)["Error"] = llvm::to_string(Err);
- Server.reply(ID, std::move(Err));
+ Reply(std::move(Err));
}
});
};
@@ -174,8 +174,47 @@
}
private:
+ // Function object to reply to an LSP call.
+ // Each instance must be called exactly once, otherwise:
+ // - the bug is logged, and (in debug mode) an assert will fire
+ // - if there was no reply, an error reply is sent
+ // - if there were multiple replies, only the first is sent
+ class ReplyOnce {
+struct State {
+ std::atomic Replied = {false};
+ json::Value ID;
+ std::string Method;
+ ClangdLSPServer *Server;
+
+ State(const json::Value &ID, StringRef Method, ClangdLSPServer *Server)
+ : ID(ID), Method(Method), Server(Server) {}
+ ~State() {
+if (!Replied) {
+ elog("No reply to message {0}({1})", Method, ID);
+ assert(false && "must reply to all calls!");
+ Server->reply(ID, make_error("server failed to reply",
+ ErrorCode::InternalError));
+}
+ }
+};
+std::shared_ptr MyState;
+
+ public:
+ReplyOnce(const json::Value &ID, StringRef Method, ClangdLSPServer *Server)
+: MyState(std::make_shared(ID, Method, Server)) {}
+
+void operator()(llvm::Expected Reply) const {
+ if (MyState->Replied.exchange(true)) {
+elog("Replied twice to message {0}({1})", MyState->Method, MyState->ID);
+assert(false && "must reply to each call only once!");
+return;
+ }
+ MyState->Server->reply(MyState->ID, std::move(Reply));
+}
+ };
+
llvm::StringMap> Notifications;
- llvm::StringMap> Calls;
+ llvm::StringMap> Calls;
// Metho
