[PATCH] D37282: clangd: Tolerate additional headers
ilya-biryukov added a comment. In https://reviews.llvm.org/D37282#860239, @puremourning wrote: > No I don't have commit rights. Can you land for me? Thanks! Done. Repository: rL LLVM https://reviews.llvm.org/D37282 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37282: clangd: Tolerate additional headers
This revision was automatically updated to reflect the committed changes.
Closed by commit rL312483: clangd: Tolerate additional headers (authored by
ibiryukov).
Repository:
rL LLVM
https://reviews.llvm.org/D37282
Files:
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
clang-tools-extra/trunk/test/clangd/protocol.test
Index: clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
===
--- clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
+++ clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
@@ -136,46 +136,68 @@
JSONRPCDispatcher ,
bool ) {
while (In.good()) {
-// A Language Server Protocol message starts with a HTTP header, delimited
-// by \r\n.
-std::string Line;
-std::getline(In, Line);
-if (!In.good() && errno == EINTR) {
- In.clear();
- continue;
+// A Language Server Protocol message starts with a set of HTTP headers,
+// delimited by \r\n, and terminated by an empty line (\r\n).
+unsigned long long ContentLength = 0;
+while (In.good()) {
+ std::string Line;
+ std::getline(In, Line);
+ if (!In.good() && errno == EINTR) {
+In.clear();
+continue;
+ }
+
+ llvm::StringRef LineRef(Line);
+
+ // We allow YAML-style comments in headers. Technically this isn't part
+ // of the LSP specification, but makes writing tests easier.
+ if (LineRef.startswith("#"))
+continue;
+
+ // Content-Type is a specified header, but does nothing.
+ // Content-Length is a mandatory header. It specifies the length of the
+ // following JSON.
+ // It is unspecified what sequence headers must be supplied in, so we
+ // allow any sequence.
+ // The end of headers is signified by an empty line.
+ if (LineRef.consume_front("Content-Length: ")) {
+if (ContentLength != 0) {
+ Out.log("Warning: Duplicate Content-Length header received. "
+ "The previous value for this message ("
+ + std::to_string(ContentLength)
+ + ") was ignored.\n");
+}
+
+llvm::getAsUnsignedInteger(LineRef.trim(), 0, ContentLength);
+continue;
+ } else if (!LineRef.trim().empty()) {
+// It's another header, ignore it.
+continue;
+ } else {
+// An empty line indicates the end of headers.
+// Go ahead and read the JSON.
+break;
+ }
}
-// Skip empty lines.
-llvm::StringRef LineRef(Line);
-if (LineRef.trim().empty())
- continue;
-
-// We allow YAML-style comments. Technically this isn't part of the
-// LSP specification, but makes writing tests easier.
-if (LineRef.startswith("#"))
- continue;
-
-unsigned long long Len = 0;
-// FIXME: Content-Type is a specified header, but does nothing.
-// Content-Length is a mandatory header. It specifies the length of the
-// following JSON.
-if (LineRef.consume_front("Content-Length: "))
- llvm::getAsUnsignedInteger(LineRef.trim(), 0, Len);
-
-// Check if the next line only contains \r\n. If not this is another header,
-// which we ignore.
-char NewlineBuf[2];
-In.read(NewlineBuf, 2);
-if (std::memcmp(NewlineBuf, "\r\n", 2) != 0)
- continue;
-
-// Now read the JSON. Insert a trailing null byte as required by the YAML
-// parser.
-std::vector JSON(Len + 1, '\0');
-In.read(JSON.data(), Len);
+if (ContentLength > 0) {
+ // Now read the JSON. Insert a trailing null byte as required by the YAML
+ // parser.
+ std::vector JSON(ContentLength + 1, '\0');
+ In.read(JSON.data(), ContentLength);
+
+ // If the stream is aborted before we read ContentLength bytes, In
+ // will have eofbit and failbit set.
+ if (!In) {
+Out.log("Input was aborted. Read only "
++ std::to_string(In.gcount())
++ " bytes of expected "
++ std::to_string(ContentLength)
++ ".\n");
+break;
+ }
-if (Len > 0) {
- llvm::StringRef JSONRef(JSON.data(), Len);
+ llvm::StringRef JSONRef(JSON.data(), ContentLength);
// Log the message.
Out.log("<-- " + JSONRef + "\n");
@@ -186,6 +208,9 @@
// If we're done, exit the loop.
if (IsDone)
break;
+} else {
+ Out.log( "Warning: Missing Content-Length header, or message has zero "
+ "length.\n" );
}
}
}
Index: clang-tools-extra/trunk/test/clangd/protocol.test
===
--- clang-tools-extra/trunk/test/clangd/protocol.test
+++ clang-tools-extra/trunk/test/clangd/protocol.test
@@ -0,0 +1,82 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# RUN: clangd -run-synchronously < %s 2>&1 | FileCheck -check-prefix=STDERR %s
+#
[PATCH] D37282: clangd: Tolerate additional headers
puremourning added a comment. In https://reviews.llvm.org/D37282#860200, @ilya-biryukov wrote: > Looks good and ready to land. Thanks for this change. > Do you have commit rights to llvm repo? Thanks for the review! No I don't have commit rights. Can you land for me? Thanks! https://reviews.llvm.org/D37282 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37282: clangd: Tolerate additional headers
ilya-biryukov added inline comments.
Comment at: clangd/JSONRPCDispatcher.cpp:167
+ // It's another header, ignore it.
continue;
+} else {
puremourning wrote:
> ilya-biryukov wrote:
> > Maybe log the skipped header?
> I think this would just be noise for conforming clients which send the
> (legitimate) Content-Type header. We could of course special case this, but
> I'm not sure the extra logging would be all that useful.
You're right. Logging `Content-Type` headers doesn't make any sense.
https://reviews.llvm.org/D37282
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37282: clangd: Tolerate additional headers
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Looks good and ready to land. Thanks for this change. Do you have commit rights to llvm repo? https://reviews.llvm.org/D37282 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37282: clangd: Tolerate additional headers
puremourning added a comment.
So I think this is ready now. Anything more you need from me?
Comment at: clangd/JSONRPCDispatcher.cpp:138
bool ) {
+ unsigned long long ContentLength = 0;
while (In.good()) {
puremourning wrote:
> ilya-biryukov wrote:
> > Maybe we could move that variable into loop body by splitting the loop body
> > into multiple parts?
> > Having it outside a loop makes it harder to comprehend the parsing code.
> > Rewriting to something like this would arguably make it easier to read:
> > ```
> > while (In.good()) {
> > // Read first line
> > //
> > llvm::StringRef LineRef = /*...*/;
> > // Skip comments
> > if (LineRef.startsWith("#"))
> > continue;
> >
> > // Read HTTP headers here, reading multiple lines right inside the loop.
> > unsigned ContentLength = 0;
> > //
> >
> >
> > // Read ContentLength bytes of a message here.
> > std::vector JSON;
> > //
> > }
> > ```
> I'll take a look, sure. Something like...
>
> ```
> while ( the input stream is good )
> {
> set len to 0
> while ( the input stream is good )
> {
> read a line into header
> if the line is a comment, read another line (continue the inner loop !)
>
> if header is Content-Length, store the length in len
> otherwise, if header is empty, we're no longer reading headers, break out
> of the inner loop
> }
>
> if the input stream is not good, break out of the loop
>
> if len is 0, reset the outer loop
>
> read len bytes from the stream,
> if we read len bytes ok, parse JSON and dispatch to the handlers
> }
> ```
done.
Comment at: clangd/JSONRPCDispatcher.cpp:163
+if (LineRef.consume_front("Content-Length: ")) {
+ llvm::getAsUnsignedInteger(LineRef.trim(), 0, ContentLength);
+ continue;
ilya-biryukov wrote:
> Maybe log an error if `Content-Length` is specified twice?
OK, though I suspect this would only happen in the tests :)
Comment at: clangd/JSONRPCDispatcher.cpp:167
+ // It's another header, ignore it.
continue;
+} else {
ilya-biryukov wrote:
> Maybe log the skipped header?
I think this would just be noise for conforming clients which send the
(legitimate) Content-Type header. We could of course special case this, but I'm
not sure the extra logging would be all that useful.
https://reviews.llvm.org/D37282
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37282: clangd: Tolerate additional headers
puremourning updated this revision to Diff 113476.
puremourning added a comment.
Validate that the duplicate message is printed.
https://reviews.llvm.org/D37282
Files:
clangd/JSONRPCDispatcher.cpp
test/clangd/protocol.test
Index: test/clangd/protocol.test
===
--- /dev/null
+++ test/clangd/protocol.test
@@ -0,0 +1,82 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# RUN: clangd -run-synchronously < %s 2>&1 | FileCheck -check-prefix=STDERR %s
+# vim: fileformat=dos
+# It is absolutely vital that this file has CRLF line endings.
+#
+# Test protocol parsing
+Content-Length: 125
+Content-Type: application/vscode-jsonrpc; charset-utf-8
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+# Test message with Content-Type after Content-Length
+#
+# CHECK: "jsonrpc":"2.0","id":0,"result":{"capabilities":{
+# CHECK-DAG: "textDocumentSync": 1,
+# CHECK-DAG: "documentFormattingProvider": true,
+# CHECK-DAG: "documentRangeFormattingProvider": true,
+# CHECK-DAG: "documentOnTypeFormattingProvider": {"firstTriggerCharacter":"}","moreTriggerCharacter":[]},
+# CHECK-DAG: "codeActionProvider": true,
+# CHECK-DAG: "completionProvider": {"resolveProvider": false, "triggerCharacters": [".",">",":"]},
+# CHECK-DAG: "definitionProvider": true
+# CHECK: }}
+
+Content-Length: 246
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"struct fake { int a, bb, ccc; int f(int i, const float f) const; };\nint main() {\n fake f;\n f.\n}\n"}}}
+
+Content-Type: application/vscode-jsonrpc; charset-utf-8
+Content-Length: 146
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:/main.cpp"},"position":{"line":3,"character":5}}}
+# Test message with Content-Type before Content-Length
+#
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[
+# CHECK-DAG: {"label":"a","kind":5,"detail":"int","sortText":"00035a","filterText":"a","insertText":"a"}
+# CHECK: ]}
+
+X-Test: Testing
+Content-Type: application/vscode-jsonrpc; charset-utf-8
+Content-Length: 146
+Content-Type: application/vscode-jsonrpc; charset-utf-8
+X-Testing: Test
+
+{"jsonrpc":"2.0","id":2,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:/main.cpp"},"position":{"line":3,"character":5}}}
+
+Content-Type: application/vscode-jsonrpc; charset-utf-8
+Content-Length: 10
+Content-Length: 146
+
+{"jsonrpc":"2.0","id":3,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:/main.cpp"},"position":{"line":3,"character":5}}}
+# Test message with duplicate Content-Length headers
+#
+# CHECK: {"jsonrpc":"2.0","id":3,"result":[
+# CHECK-DAG: {"label":"a","kind":5,"detail":"int","sortText":"00035a","filterText":"a","insertText":"a"}
+# CHECK: ]}
+# STDERR: Warning: Duplicate Content-Length header received. The previous value for this message (10) was ignored.
+
+Content-Type: application/vscode-jsonrpc; charset-utf-8
+Content-Length: 10
+
+{"jsonrpc":"2.0","id":4,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:/main.cpp"},"position":{"line":3,"character":5}}}
+# Test message with malformed Content-Length
+#
+# STDERR: JSON dispatch failed!
+# Ensure we recover by sending another (valid) message
+
+Content-Length: 146
+
+{"jsonrpc":"2.0","id":5,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:/main.cpp"},"position":{"line":3,"character":5}}}
+# Test message with Content-Type before Content-Length
+#
+# CHECK: {"jsonrpc":"2.0","id":5,"result":[
+# CHECK-DAG: {"label":"a","kind":5,"detail":"int","sortText":"00035a","filterText":"a","insertText":"a"}
+# CHECK: ]}
+
+Content-Length: 1024
+
+{"jsonrpc":"2.0","id":5,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:/main.cpp"},"position":{"line":3,"character":5}}}
+# Test message which reads beyond the end of the stream.
+#
+# Ensure this is the last test in the file!
+# STDERR: Input was aborted. Read only {{[0-9]+}} bytes of expected {{[0-9]+}}.
+
Index: clangd/JSONRPCDispatcher.cpp
===
--- clangd/JSONRPCDispatcher.cpp
+++ clangd/JSONRPCDispatcher.cpp
@@ -136,46 +136,68 @@
JSONRPCDispatcher ,
bool ) {
while (In.good()) {
-// A Language Server Protocol message starts with a HTTP header, delimited
-// by \r\n.
-std::string Line;
-std::getline(In, Line);
-if (!In.good() && errno == EINTR) {
- In.clear();
- continue;
+// A Language Server Protocol message starts with a set of HTTP headers,
+// delimited by \r\n, and terminated by an empty line (\r\n).
+unsigned long long ContentLength = 0;
+while (In.good()) {
+ std::string Line;
+ std::getline(In, Line);
+ if
