[PATCH] D114415: [llvm] [Support] Add HTTP Server Support library.

[Noah Shutty via Phabricator via llvm-commits]

noajshu added a comment.

Hi @dblaikie thank you so much for the review! I believe I have addressed all your comments, however they disappeared because I moved the HTTPServer into the Debuginfod library. You can see them on  the previous revision <https://reviews.llvm.org/D114415?id=390457>.



================
Comment at: llvm/lib/Support/HTTPServer.cpp:101-102
+                     if (Offset < Response.ContentLength) {
+                       std::string Chunk = Response.Provider(Offset, Length);
+                       Sink.write(Chunk.c_str(), Chunk.size());
+                     }
----------------
dblaikie wrote:
> Looks like this could use StringRef instead of std::string, to avoid copying the contents? Or are there cases where a provider might want to return content by value, rather than from some underlying/longer-lived storage? (worth providing something that could be optimal for both cases somehow - like providing an optional std::string out parameter that the provider could populate if it doesn't have its own long-lived backing store?)
I've switched it to take a StringRef -- for the only place this is used currently (streaming chunks of a file) you have a persistent MemoryBuffer which is responsible for the storage. 



================
Comment at: llvm/lib/Support/HTTPServer.cpp:125-130
+Expected<unsigned> HTTPServer::bindAny(StringRef HostInterface) {
+  SmallString<16> HostInterfaceStorage;
+  StringRef S =
+      Twine(HostInterface).toNullTerminatedStringRef(HostInterfaceStorage);
+
+  int ListenPort = Server.bind_to_any_port(S.begin());
----------------
dblaikie wrote:
> If the underlying API requires a c-style string, maybe simpler to expose that up through the caller layers? Rather than allocating a buffer to create a null terminated string when the caller might already have one to pass down anyway? (alternatively, I guess, if you use Twine as the parameter type rather than StringRef, then at least in the cases where the Twine does refer to a null terminated string it won't have to copy the string again - currently since the HostInterface is StringRef, it'd always have to copy into the buffer and append a null terminator)
It's a good point that no matter whether we use cpp-httplib or something else, ultimately the system call will need a c-style string. I've changed it to a `const char*` and I could switch to a Twine if it's better.


================
Comment at: llvm/lib/Support/HTTPServer.cpp:142-144
+    return createStringError(
+        errc::io_error,
+        "An unknown error occurred when cpp-httplib attempted to listen.");
----------------
dblaikie wrote:
> Follow-up work to use httplib set_error_handler to improve these error messages, perhaps?
Good idea!
It looks like httplib's `set_error_handler` is used to set the default response content (e.g. an error page) when the user's handler [[ https://github.com/yhirose/cpp-httplib/blob/master/httplib.h#L4854 | returns an HTTP error code ]] (>400). More detailed error information is available to us by checking `errno`, but cpp-httplib doesn't check it other than to see if the bind / listen was successful.

In a future TCPSocket implementation for LLVM it would be great to parse the `errno` better to give more helpful messages to the user.


================
Comment at: llvm/unittests/Support/HTTPServer.cpp:82
+  EXPECT_EQ(Buffer.Code, Response.Code);
+  EXPECT_EQ(Buffer.Body->MemoryBuffer::getBuffer(), Response.Body);
+  Server.stop();
----------------
dblaikie wrote:
> The `MemoryBuffer::` probably isn't needed here, I'd have thought?
Ah, sadly it is required, unless you can see a better way around the following.
The `Body` is a `WritableMemoryBuffer`, whose override of `getBuffer` returns an `MutableArrayRef<char>` (as opposed to the parent's virtual method `MemoryBuffer::getBuffer` which returns a `StringRef`). For some reason there is no `operator==` implemented between `MutableArrayRef<char>` and `std::string`. So we have to manually specify use of the parent method


================
Comment at: llvm/unittests/Support/HTTPServer.cpp:184-189
+  // Timeout below 50ms, request should fail
+  Client.setTimeout(40);
+  Expected<HTTPResponseBuffer> BufferOrErr = Client.get(Url);
+  EXPECT_THAT_EXPECTED(BufferOrErr, Failed<StringError>());
+  // Timeout above 50ms, request should succeed
+  Client.setTimeout(60);
----------------
dblaikie wrote:
> timing tests like this can be sensitive to machine load - perhaps setting the acceptable timeout much higher than the actual timeout would be good to increase test stability? (eg: the 40 is probably fine - the actual 50ms delay shouldn't ever be shorter than requested, and so it probably shouldn't tickle that test (though it's possible - if the test code were delayed in reaching the timeout call while the delay was already running, it could appear as though the delay was shorter than expected) - but changing the 60 up to something well above the threshold might be good).
> 
> Though also, it's not your job to test that HTTPClient implements timeouts correctly, only that you pass them down through the API - so if there's a more robust way to test that property without actually having to run real timeouts (or at least not testing them so precisely) it might be helpful.
This is a really good observation. I'm concerned that even with a lot of extra wait time, it might flake under some extreme system load. Let's remove it for now, leaving just the test in which a timeout is expected.
`HTTPClient::setTimeout` is just directly calling `curl_easy_setopt` so I think that the libcurl tests should cover it for the case where a (nonzero) timeout does not expire.


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

https://reviews.llvm.org/D114415