[llvm] [Support] Add HTTP Client Support library.

[noajshu]

Summary

This patch implements a small HTTP client library consisting primarily of the HTTPRequest, HTTPResponseHandler, and BufferedHTTPResponseHandler classes. Unit tests of the HTTPResponseHandler and BufferedHTTPResponseHandler are included.

[noajshu]

Remove extraneous curl.h include.

[noajshu]

Refactor to three primary parent classes HTTPClient, HTTPRequest, HTTPResponseHandler.
This is a more straightforward design that presents a simpler usage pattern. E.g., we can do this:

Expected<HTTPResponseBuffer> ResponseOrErr = Client.get("http://example.com");
if (Error Err = ResponseOrErr)
    return Err;
outs() << ResponseOrErr->Body;

[noajshu]

Improve doxygen comments and formatting.

[noajshu]

Refactor to isolate most logic in HTTPResponseHandler.

[noajshu]

@labath thank you for your comments on the previous version of this diff before splitting!
I have refactored and simplified things, so that we have three main classes:

• HTTPClient
• HTTPRequest
• HTTPResponseHandler

This diff just adds the skeleton, while the next in the sequence (D112753) adds the CURL client itself. Do you think the base HTTPClient should just use curl by default? In that case perhaps these diffs could be merged.

By the way, I wanted to share some of the motivation for the added complexity of these three classes instead of just an httpGet function.
I am thinking ahead to their use in the debuginfod client and server. The HTTPClient and HTTPResponseHandler can be extended for concurrent requests or streaming response handlers, which both could be used for debuginfod.
The client could use concurrent requests to send a HEAD to all N known server URLs at once, rather than serially requesting the asset from each server. Large debugging assets could be processed piece-by-piece using a StreamingHTTPResponseHandler. This would be used during federation by a debuginfod server, enabling immediate write of data chunks received from the peer server back to the client.
Curl’s docs say that re-use of handles is key to good performance and this is part of the reason to have a Client that reuses a handle when running multiple requests. However I haven't measured this performance difference myself.

[labath]

I've found the error handling confusing. If the intention is that the HTTPResponseHandler can terminate the request by returning zero, then it seems to me that it would be better to have it return llvm::Error, and have the HTTPClient class translate that to whatever action is required ( == storing the error in some variable and returning 0 to curl). That would make the BufferedHTTPResponse class cleaner, as it would no longer have to deal with an Error member, and the overall api would be more understandable.

llvm/include/llvm/Support/HTTPClient.h

41

Should this be a period, or is something missing?

45

I'm having trouble reconciling this with the "cannot fail" comment above. Can you elaborate on what the two comments mean?

71–76

Having an Error member is strange enough (because of it's destruction semantics), but making it a public member is unheard of. It would be much better to make these private and have a member function returning Expected<HTTPResponseBuffer>.

But it would be much better to not have the Error member in the first place.

95

I don't see any methods returning an error

llvm/lib/Support/HTTPClient.cpp

51

if you used llvm::to_integer here, you would be able to use any type you want (size_t perhaps), and would get more natural failure semantics (no need for ! -- to_integer returns true on success).

llvm/unittests/Support/HTTPClient.cpp

43–46

EXPECT_EQ(Handler.ResponseBuffer.Body->getBuffer(), "...");

158–163

What is this supposed to test? This test pretty much only executes code which is defined in the test itself...

[noajshu]

@labath I completely agree the new error handling is awkward and I prefer your proposed Error handling between the ResponseHandler and the Client. I'll change all three handle*() methods to return Error (then convert it to 0 for curl in the client + hook functions)

llvm/unittests/Support/HTTPClient.cpp

43–46

Thanks! Because the Body is actually a WritableMemoryBuffer, I need an extra namespace qualifier on getBuffer for this to work.

On that note -- do you think I should change the HTTP response body to be a MemoryBuffer (read-only)?

[noajshu]

llvm/lib/Support/HTTPClient.cpp

51

Ah, the perfect fit. Thanks!

llvm/unittests/Support/HTTPClient.cpp

158–163

I agree and I will remove the mockHTTPClientTest.

Do you think there is any value in the BrokenConnection test? I would be happy to remove this test as well. The only non-test code executed in that one is Client.perform(Request) and Client.get("") which is effectively a change-detector for the short convenience methods of the Client.

[noajshu]

Update error handling of HTTPResponseHandler and HTTPClient

[labath]

This looks good to me now, modulo some details in inline comments.

llvm/lib/Support/HTTPClient.cpp

30–32

= default;

llvm/unittests/Support/HTTPClient.cpp

18

This block looks weird -- I'd just make a separate test for that.

43–46

I don't see a need for that -- I can see how the caller might want to modify the buffer for its own purposes. and he can always convert it to a read-only buffer if he does not (the opposite is harder).

158–163

Yeah, I'd probably remove them, or at least significantly simplify them. If you want to test that the virtual perform is getting called, then all you need to do is mock that method -- you don't really need to provide a fake implementation of that method, and everything that gets called by it...

[noajshu]

Address final comments.

[labath]

I think this is as good as it can get. What you do after that -- I don't know.

It wasn't clear to me whether you forgot about the mockHTTPClientBrokenConnectionTest part, or chose not to do that, but I added an additional comment to describe how I would do this.

llvm/lib/Support/HTTPClient.cpp

30

= default

llvm/unittests/Support/HTTPClient.cpp

141–142

MOCK_METHOD2(perform, Error(HTTPRequest, HTTPResponseHandler)), and then just set expectations in the test. The entire MockHTTPResponseHandler business can go away.

[noajshu]

Make perform(Request, Handler) non-virtual, adopting D112753's convention for HTTPClient implementation.

[noajshu]

nit, improve comment

[noajshu]

Make HTTPClient::isAvailable static.

[noajshu]

In D112751#3111697, @labath wrote:

I think this is as good as it can get. What you do after that -- I don't know.

It wasn't clear to me whether you forgot about the mockHTTPClientBrokenConnectionTest part, or chose not to do that, but I added an additional comment to describe how I would do this.

Thanks Pavel! I decided to remove the mock client tests, because they only hit the few-line convenience methods. I'm happy to add them, I'm just unsure how to avoid making HTTPClient::perform(Request, Handler) virtual. In any case they will be hit in subsequent testing after a working client implementation (i.e. Curl in D112753) is added.

llvm/unittests/Support/HTTPClient.cpp

141–142

Thanks! It seems the only library code we hit with these tests is the small convenience methods perform(Request) and get(Url). Is it worth making the perform method virtual to be able to create the mock class?

[noajshu]

Final style updates.

[noajshu]

Remove extraneous #define DEBUG_TYPE

[labath]

llvm/unittests/Support/HTTPClient.cpp

141–142

Probably not. It made sense while curl was implemented in a separate class, but I don't think it's worth going out of our way to make this work.

[dblaikie]

Couple of minor API design issues, but haven't dug in much.

@labath - what's your state on this and the subsequent patches? What would be best for me to focus on/do here to help out?

llvm/lib/Support/HTTPClient.cpp

26

Twine's mostly meant for when a string value may only be conditionally manifest (such as in a logging library where some dynamic log level would dictate whether the value is logged) - if the string is unconditionally manifest, Twine probably isn't the right tool to use here.

You could pass a SmallString by value & move it into the Url, but since it's a smallstring, it has a big sizeof and might not be the most efficient thing to move around. Alternatively just pass a StringRef.

(similar feedback up the stack/for other uses of Twine)

llvm/unittests/Support/HTTPClient.cpp

65–72

If this code isn't touching/implementing to_integer, it's probably not necessary/appropriate to re-test it here, should just rely on its existing definition/documentation/testing elsewhere.

86

Perhaps avoid the hardcoded 19 somehow? Like you could declare the string as a named char array, then use sizeof to get the length (-1 for the implicit null terminator, if that's not meant to be included)

[noajshu]

Respond to feedback from @dblaikie.

[noajshu]

Thanks for the comments @dblaikie! I've converted to StringRef parameters throughout.

[labath]

In D112751#3119625, @dblaikie wrote:

@labath - what's your state on this and the subsequent patches? What would be best for me to focus on/do here to help out?

I've pretty much done all I can with this series. I am happy with the code quality. Test coverage is ok. There's one ongoing discussion in https://reviews.llvm.org/D112753#inline-1083341, that I'd like to hear your thoughts on (if you have any), but that's a fairly small detail.

I'd say the main thing that's missing is for someone to say "this belongs in llvm".

[noajshu]

Move global curl initialization / cleanup to static HTTPClient::initialize / HTTPClient::cleanup.

[dblaikie]

In D112751#3120753, @labath wrote:

In D112751#3119625, @dblaikie wrote:

@labath - what's your state on this and the subsequent patches? What would be best for me to focus on/do here to help out?

I've pretty much done all I can with this series. I am happy with the code quality. Test coverage is ok. There's one ongoing discussion in https://reviews.llvm.org/D112753#inline-1083341, that I'd like to hear your thoughts on (if you have any), but that's a fairly small detail.

I'd say the main thing that's missing is for someone to say "this belongs in llvm".

Fair enough - Yeah, I think the direction's worth having a go at - be nice to have it supported in llvm-symbolizer, etc.

llvm/include/llvm/Support/HTTPClient.h

32

Usually best to write op overloads as non-members (possibly as friends, if that's necessary/helpful) to avoid different conversion candidates an the LHS and RHS.

36

Skip const here

108

Skip top-level const here - while it's generally out of character for LLVM code, it's also not part of the function declaration (const here doesn't change the type of the function/doesn't change how people call it, it's only an implementation detail)

llvm/lib/Support/HTTPClient.cpp

26

Skip top-level "const" here - it's out of character for LLVM code.

75

skip const here

80–82

Use = default; here, perhaps?

86–90

Is there any use for this variable? It's currently only written and never read, by the looks of it. Maybe that's worth deferring until some use is implemented.

[noajshu]

Remove unnecessary IsInitialized variable. Also remove some const qualifiers and replace empty ctor/dtor with =default

[noajshu]

Small style/formatting update.

[noajshu]

Add HTTPClient::setTimeout method.

[dblaikie]

llvm/unittests/Support/HTTPClient.cpp

76

This probably doesn't test what it's intended to test - the string will terminate at the first '\0' - if you want to test embedded null characters, you'd need to specify an explicit length when creating the StringRef explicitly, rather than relying on StringRef(const char*) conversion.

[noajshu]

String literal fix for unit test with embedded null characters.

llvm/unittests/Support/HTTPClient.cpp

76

Thanks for pointing this out! I have added a fix using C++14 string literals. If you prefer I can also change this to use a hardcoded string length instead.

[dblaikie]

llvm/lib/Support/HTTPClient.cpp

74–75

Looks like this API doesn't require the HTTPResponseHandler to have a virtual dtor - if these objects will usually be owned directly, rather than indirectly through a unique_ptr of the base type, etc, then the dtor can be non-virtual (but the base type will have to make the dtor protected and concrete derived classes would need to be marked final)

llvm/unittests/Support/HTTPClient.cpp

76

Oh, great!

If the string data doesn't need to outlive the handleHeaderLine call, you could roll it in:

...handleHeaderLine("Content-Length: \0\0\0"s),
...

Rather than having a standalone named variable. Though if you prefer the named variable to document the code intent, that's totally fair too!

[noajshu]

Change HTTPRequestHandler dtor from virtual to protected.

[noajshu]

Thanks for the tip on the protected dtor! I also changed the setTimeout to use std::chrono::milliseconds following the discussion on D112758.

[noajshu]

remove extraneous semicolon

[dblaikie]

Generally (still) looks good to me.

llvm/include/llvm/Support/HTTPClient.h

+//===-- llvm/Support/HTTPClient.h - HTTP client library ---*- C++ -------*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// This file contains the declarations of the HTTPClient, HTTPMethod,
+/// HTTPResponseHandler, and BufferedHTTPResponseHandler classes, as well as
+/// the HTTPResponseBuffer and HTTPRequest structs.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_SUPPORT_HTTP_CLIENT_H
+#define LLVM_SUPPORT_HTTP_CLIENT_H
+
+#include "llvm/Support/Error.h"
+#include "llvm/Support/MemoryBuffer.h"
+
+namespace llvm {
+
+enum class HTTPMethod { GET };
+
+/// A stateless description of the configuration of an outbound HTTP Request.
+/// An HTTPRequest can be performed any number of times by HTTPClient::perform.
+struct HTTPRequest {
+  SmallString<128> Url;
+  HTTPMethod Method = HTTPMethod::GET;
+  bool FollowRedirects = true;
+  bool operator==(const HTTPRequest &O) const {
+    return Url == O.Url && Method == O.Method &&
+           FollowRedirects == O.FollowRedirects;
+  }
+  HTTPRequest(const Twine &Url);
+};
+
+/// A handler for state updates during the lifecycle of a request performed
+/// by HTTPClient::perform.
+class HTTPResponseHandler {
+public:
+  /// Processes one line of HTTP response headers. If an Error is returned the
+  /// caller should abort the request.
+  virtual Error handleHeaderLine(StringRef HeaderLine) = 0;
+
+  /// Processes an additional chunk of bytes of the HTTP response body. If an
+  /// Error is returned the caller should abort the request.
+  virtual Error handleBodyChunk(StringRef BodyChunk) = 0;
+
+  /// Processes the HTTP response status code. If an Error is returned the
+  /// caller should abort the request.
+  virtual Error handleStatusCode(unsigned Code) = 0;
+
+  virtual ~HTTPResponseHandler();
+};
+
+/// An HTTP response status code bundled with a buffer to store the body.
+struct HTTPResponseBuffer {
+  unsigned Code = 0;
+  std::unique_ptr<WritableMemoryBuffer> Body;
+};
+
+/// A simple handler which writes returned data to an HTTPResponseBuffer.
+/// Discards all headers except the Content-Length, which it uses to
+/// allocate an appropriately-sized Body buffer.
+class BufferedHTTPResponseHandler : public HTTPResponseHandler {
+  size_t Offset = 0;
+
+public:
+  /// Stores the data received from the HTTP server.
+  HTTPResponseBuffer ResponseBuffer;
+
+  /// Implementations of the HTTPResponseHandler functions to store the
+  /// HTTP response in a buffer allocated based on Content-Length.
+  Error handleHeaderLine(StringRef HeaderLine) override;
+  Error handleBodyChunk(StringRef BodyChunk) override;
+  Error handleStatusCode(unsigned Code) override;
+};
+
+/// A reusable client for performing outbound HTTP requests.
+class HTTPClient {
+public:
+  /// Checks whether HTTPClient::perform works in this build.
+  bool isAvailable();
+
+  /// Performs the Request and passes response data to the Handler. Returns any
+  /// errors which occur during request. If any of the callbacks to Handler
+  /// return an Error, a Client should abort the request and return an Error.
+  Error perform(const HTTPRequest &Request, HTTPResponseHandler &Handler);
+
+  /// Performs the Request with the default BufferedHTTPResponseHandler, and
+  /// returns its HTTPResponseBuffer or an Error.
+  Expected<HTTPResponseBuffer> perform(const HTTPRequest &Request);
+
+  /// Performs an HTTPRequest with the default configuration to make a GET
+  /// request to the given Url. Returns an HTTPResponseBuffer or an Error.
+  Expected<HTTPResponseBuffer> get(const Twine &Url);
+};
+
+} // end namespace llvm
+
+#endif // LLVM_SUPPORT_HTTP_CLIENT_H

llvm/lib/Support/CMakeLists.txt

add_llvm_component_library(LLVMSupport
   FileUtilities.cpp
   FileOutputBuffer.cpp
   FoldingSet.cpp
   FormattedStream.cpp
   FormatVariadic.cpp
   GlobPattern.cpp
   GraphWriter.cpp
   Hashing.cpp
+  HTTPClient.cpp
   InitLLVM.cpp
   InstructionCost.cpp
   IntEqClasses.cpp
   IntervalMap.cpp
   ItaniumManglingCanonicalizer.cpp
   JSON.cpp
   KnownBits.cpp
   LEB128.cpp

llvm/lib/Support/HTTPClient.cpp

+//===-- llvm/Support/HTTPClient.cpp - HTTP client library -------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+///
+/// This file defines the methods of the HTTPRequest, HTTPClient, and
+/// BufferedHTTPResponseHandler classes.
+///
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/HTTPClient.h"
+#include "llvm/ADT/APInt.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/MemoryBuffer.h"
+
+using namespace llvm;
+
+#define DEBUG_TYPE "HTTPClient"
+
+HTTPRequest::HTTPRequest(const Twine &Url) { this->Url = Url.str(); }
+
+HTTPResponseHandler::~HTTPResponseHandler() = default;
+
+Expected<HTTPResponseBuffer> HTTPClient::perform(const HTTPRequest &Request) {
+  BufferedHTTPResponseHandler Handler;
+  if (Error Err = perform(Request, Handler))
+    return std::move(Err);
+  return std::move(Handler.ResponseBuffer);
+}
+
+Expected<HTTPResponseBuffer> HTTPClient::get(const Twine &Url) {
+  HTTPRequest Request(Url);
+  return perform(Request);
+}
+
+static inline bool parseContentLengthHeader(StringRef LineRef,
+                                            size_t &ContentLength) {
+  // Content-Length is a mandatory header, and the only one we handle.
+  return LineRef.consume_front("Content-Length: ") &&
+         to_integer(LineRef.trim(), ContentLength, 10);
+}
+
+Error BufferedHTTPResponseHandler::handleHeaderLine(StringRef HeaderLine) {
+  if (ResponseBuffer.Body)
+    return Error::success();
+
+  size_t ContentLength;
+  if (parseContentLengthHeader(HeaderLine, ContentLength))
+    ResponseBuffer.Body =
+        WritableMemoryBuffer::getNewUninitMemBuffer(ContentLength);
+
+  return Error::success();
+}
+
+Error BufferedHTTPResponseHandler::handleBodyChunk(StringRef BodyChunk) {
+  if (!ResponseBuffer.Body)
+    return createStringError(errc::io_error,
+                             "Unallocated response buffer -- HTTP Body data "
+                             "recieved before Content-Length Header.");
+  if (Offset + BodyChunk.size() > ResponseBuffer.Body->getBufferSize())
+    return createStringError(errc::io_error,
+                             "Content is larger than response buffer.");
+  memcpy(ResponseBuffer.Body->getBufferStart() + Offset, BodyChunk.data(),
+         BodyChunk.size());
+  Offset += BodyChunk.size();
+  return Error::success();
+}
+
+Error BufferedHTTPResponseHandler::handleStatusCode(unsigned Code) {
+  ResponseBuffer.Code = Code;
+  return Error::success();
+}
+
+bool HTTPClient::isAvailable() { return false; }
+Error HTTPClient::perform(const HTTPRequest &Request,
+                          HTTPResponseHandler &Handler) {
+  llvm_unreachable("No HTTP Client implementation available.");
+}

llvm/unittests/Support/CMakeLists.txt

add_llvm_unittest(SupportTests
   FileCollectorTest.cpp
   FileOutputBufferTest.cpp
   FileUtilitiesTest.cpp
   FormatVariadicTest.cpp
   FSUniqueIDTest.cpp
   GlobPatternTest.cpp
   HashBuilderTest.cpp
   Host.cpp
+  HTTPClient.cpp
   IndexedAccessorTest.cpp
   InstructionCostTest.cpp
   ItaniumManglingCanonicalizerTest.cpp
   JSONTest.cpp
   KnownBitsTest.cpp
   LEB128Test.cpp
   LinearPolyBaseTest.cpp
   LineIteratorTest.cpp

llvm/unittests/Support/HTTPClient.cpp

+//===-- llvm/unittest/Support/HTTPClient.cpp - unit tests -------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/HTTPClient.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+TEST(HTTPClientTests, bufferedHTTPResponseHandlerNoContentLengthLifecycleTest) {
+  BufferedHTTPResponseHandler Handler;
+  // test initial state
+  EXPECT_EQ(Handler.ResponseBuffer.Code, 0u);
+  EXPECT_EQ(Handler.ResponseBuffer.Body, nullptr);
+
+  // A body chunk passed before the content-length header is an error.
+  EXPECT_THAT_ERROR(Handler.handleBodyChunk("body"),
+                    Failed<llvm::StringError>());
+  EXPECT_THAT_ERROR(Handler.handleHeaderLine("a header line"), Succeeded());
+  EXPECT_THAT_ERROR(Handler.handleBodyChunk("body"),
+                    Failed<llvm::StringError>());
+}
+
+TEST(HTTPClientTests, bufferedHTTPResponseHandlerLifecycleTest) {
+  BufferedHTTPResponseHandler Handler;
+  EXPECT_THAT_ERROR(Handler.handleHeaderLine("Content-Length: 36\r\n"),
+                    Succeeded());
+
+  EXPECT_THAT_ERROR(Handler.handleBodyChunk("body:"), Succeeded());
+  EXPECT_THAT_ERROR(Handler.handleBodyChunk("this puts the total at 36 chars"),
+                    Succeeded());
+  EXPECT_EQ(Handler.ResponseBuffer.Body->MemoryBuffer::getBuffer(),
+            "body:this puts the total at 36 chars");
+
+  // additional content should be rejected by the handler
+  EXPECT_THAT_ERROR(
+      Handler.handleBodyChunk("extra content past the content-length"),
+      Failed<llvm::StringError>());
+
+  // test response code handling
+  EXPECT_THAT_ERROR(Handler.handleStatusCode(200u), Succeeded());
+  EXPECT_EQ(Handler.ResponseBuffer.Code, 200u);
+  EXPECT_THAT_ERROR(Handler.handleStatusCode(400u), Succeeded());
+  EXPECT_EQ(Handler.ResponseBuffer.Code, 400u);
+}
+
+TEST(HTTPClientTests, bufferedHTTPResponseHandlerZeroContentLengthTest) {
+  BufferedHTTPResponseHandler Handler;
+  EXPECT_THAT_ERROR(Handler.handleHeaderLine("Content-Length: 0"), Succeeded());
+  EXPECT_NE(Handler.ResponseBuffer.Body, nullptr);
+  EXPECT_EQ(Handler.ResponseBuffer.Body->getBufferSize(), 0u);
+
+  // all content should be rejected by the handler
+  EXPECT_THAT_ERROR(Handler.handleBodyChunk("non-empty body content"),
+                    Failed<llvm::StringError>());
+}
+
+TEST(HTTPClientTests, bufferedHTTPResponseHandlerMalformedContentLengthTest) {
+  // confirm llvm::to_integer behaves as expected
+  size_t ContentLength;
+  EXPECT_EQ(llvm::to_integer("fff", ContentLength, 10), false);
+  EXPECT_EQ(llvm::to_integer("", ContentLength, 10), false);
+  EXPECT_EQ(llvm::to_integer("-1", ContentLength, 10), false);
+  EXPECT_EQ(llvm::to_integer("0111", ContentLength, 10), true);
+  EXPECT_EQ(llvm::to_integer("0", ContentLength, 10), true);
+  EXPECT_EQ(ContentLength, 0u);
+
+  // check that several invalid content lengths are ignored
+  BufferedHTTPResponseHandler Handler;
+  EXPECT_EQ(Handler.ResponseBuffer.Body, nullptr);
+  EXPECT_THAT_ERROR(Handler.handleHeaderLine("Content-Length: fff"),
+                    Succeeded());
+  EXPECT_EQ(Handler.ResponseBuffer.Body, nullptr);
+
+  EXPECT_THAT_ERROR(Handler.handleHeaderLine("Content-Length:    "),
+                    Succeeded());
+  EXPECT_EQ(Handler.ResponseBuffer.Body, nullptr);
+
+  EXPECT_THAT_ERROR(
+      Handler.handleHeaderLine(StringRef("Content-Length: \0\0\0", 19)),
+      Succeeded());
+  EXPECT_EQ(Handler.ResponseBuffer.Body, nullptr);
+
+  EXPECT_THAT_ERROR(Handler.handleHeaderLine("Content-Length: -11"),
+                    Succeeded());
+  EXPECT_EQ(Handler.ResponseBuffer.Body, nullptr);
+
+  // all content should be rejected by the handler because
+  // no valid content length has been received
+  EXPECT_THAT_ERROR(Handler.handleBodyChunk("non-empty body content"),
+                    Failed<llvm::StringError>());
+}