clangd: Tolerate additional headers

[puremourning]

Summary

The language server protocol specified 2 headers (Content-Length and Content-Type), but does not specify their sequence. It specifies that an empty line ends
headers. Clangd has been updated to handle arbitrary sequences of headers, extracting only the content length.

[ilya-biryukov]

Thanks for the fix. Looks good, just a few comments.

clangd/JSONRPCDispatcher.cpp

138 ↗

(On Diff #113175)

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<char> JSON;
  //....
}

156 ↗

(On Diff #113175)

I believe that FIXME referred to the fact that the header does nothing in clangd.
For example, one important part of Content-Type is charset, we completely ignore that now.
So we should probably leave a FIXME, maybe rephrasing it a bit.

@bkramer, could you confirm I got the meaning of the comment right?

163 ↗

(On Diff #113175)

Maybe log an error if Content-Length is specified twice?

167 ↗

(On Diff #113175)

Maybe log the skipped header?

202 ↗

(On Diff #113175)

Maybe log the ignored line here?

test/clangd/protocol.test

61 ↗

(On Diff #113175)

Maybe also check stderr by piping it into stdout and using a separate -check-prefix?
I mean something like:

# RUN: clangd -run-synchronously < %s | FileCheck %s
# RUN: clangd -run-synchronously < %s  2>&1 >/dev/null | FileCheck -check-prefix=ERR %s
# ...
# ERR: JSON dispatch failed!

[puremourning]

Thanks for the review! Few quick responses.

clangd/JSONRPCDispatcher.cpp

138 ↗

(On Diff #113175)

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
}

156 ↗

(On Diff #113175)

Ah that's a good point. I had assumed the FIXME was that header parsing wasn't complete.

OTOH, the language server protocol states:

The content part is encoded using the charset provided in the Content-Type field. It defaults to utf-8, which is the only encoding supported right now.

So it's all a little moot :)

205 ↗

(On Diff #113175)

typo "consumed"

test/clangd/protocol.test

61 ↗

(On Diff #113175)

Ah neat. I did try and work out a way to do that. Will do this, thanks for the tip.

[puremourning]

Keep ContentLength within the loop by using an inner loop to read headers.
Update tests to check stderr correctly.
Add some useful diagnostics to the log.

[puremourning]

Tidy logging: missing newlines.

[puremourning]

Validate that the duplicate message is printed.

[puremourning]

So I think this is ready now. Anything more you need from me?

clangd/JSONRPCDispatcher.cpp

138 ↗

(On Diff #113175)

done.

163 ↗

(On Diff #113175)

OK, though I suspect this would only happen in the tests :)

167 ↗

(On Diff #113175)

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.

[ilya-biryukov]

Looks good and ready to land. Thanks for this change.
Do you have commit rights to llvm repo?

[ilya-biryukov]

clangd/JSONRPCDispatcher.cpp

167 ↗

(On Diff #113175)

You're right. Logging Content-Type headers doesn't make any sense.

[puremourning]

In 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!

[ilya-biryukov]

In D37282#860239, @puremourning wrote:

No I don't have commit rights. Can you land for me? Thanks!

Done.

clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp

bool JSONRPCDispatcher::call(StringRef Content) const {
 
   return true;
 }
 
 void clangd::runLanguageServerLoop(std::istream &In, JSONOutput &Out,
                                    JSONRPCDispatcher &Dispatcher,
                                    bool &IsDone) {
   while (In.good()) {
-    // A Language Server Protocol message starts with a HTTP header, delimited
-    // by \r\n.
+    // 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;
       }
 
-    // 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.
+      // 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;
 
-    unsigned long long Len = 0;
-    // FIXME: Content-Type is a specified header, but does nothing.
+      // 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);
+      // 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");
+        }
 
-    // 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)
+        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;
+      }
+    }
 
+    if (ContentLength > 0) {
       // Now read the JSON. Insert a trailing null byte as required by the YAML
       // parser.
-    std::vector<char> JSON(Len + 1, '\0');
-    In.read(JSON.data(), Len);
+      std::vector<char> 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");
 
       // Finally, execute the action for this JSON message.
       if (!Dispatcher.call(JSONRef))
         Out.log("JSON dispatch failed!\n");
 
       // If we're done, exit the loop.
       if (IsDone)
         break;
+    } else {
+      Out.log( "Warning: Missing Content-Length header, or message has zero "
+               "length.\n" );
     }
   }
 }

clang-tools-extra/trunk/test/clangd/protocol.test

+# 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]+}}.
+