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

[Noah Shutty via Phabricator via llvm-commits]

noajshu marked 6 inline comments as done.
noajshu added inline comments.


================
Comment at: llvm/lib/Support/HTTPServer.cpp:58
+          },
+          [=](bool Success) { delete MB; }};
+}
----------------
dblaikie wrote:
> Could this parameter (the last lambda, the "CompletionHandler" member in StreamingHTTPResponse) be move-only (non-copyable) in that case it could capture-by-move the std::unique_ptr<MemoryBuffer> and be a bit more robust in terms of memory management?
That's a great idea! Cpp-httplib converts the content provider to a [[ https://github.com/yhirose/cpp-httplib/blob/master/httplib.h#L336 | ContentProvider ]] which is a std::function. This ends up copying the lambda.

I was thinking we might refactor the interface, so instead of returning the response (or response provider) you can interact with a "HTTPServerRequest" object similar to how cpp-httplib's own API works.
Their API is more flexible. For example, you can decide at runtime whether you want to do streaming response handling or return a single response string. In the wrapper implementation here you have to either return a StreamingHTTPResponse or an HTTPResponse, fixed at compile time.

Even if we mirror the httplib interface, the problem remains that we cannot pass a non-copyable lambda directly to httplib's `set_content_provider`. I'm not sure if there is a nice way around this problem.


================
Comment at: llvm/lib/Support/HTTPServer.cpp:67
+
+Error HTTPServer::get(StringRef UrlPathPattern, HTTPRequestHandler Handler) {
+  std::string ErrorMessage;
----------------
dblaikie wrote:
> Perhaps HTTPRequestHandler should be an `llvm::function_ref` since it doesn't appear to outlive this call, if I'm understanding the code correctly?
Ah, I think the nomenclature is misleading.  `HTTPServer::get` should be called something like `HTTPServer::registerGetHandler`. So it returns, and then you can later call `bind()` etc., but the function provided in the argument may be called much later after `listen` when there is a request to be handled.


================
Comment at: llvm/unittests/Support/HTTPServer.cpp:77
+  std::string Url = "http://localhost:" + utostr(Port);
+  HTTPClient::initialize();
+  Expected<HTTPResponseBuffer> BufferOrErr = HTTPClient().get(Url);
----------------
dblaikie wrote:
> Is some corresponding de-initialize/teardown required/desirable? (perhaps some scoped initialize/de-initialize) or put the initialize in a global ctor (or test fixture (SetUp? Whatever the one is that's called before any test (but not before every test individually)) in the test file if there's no expectation/desire to initialize/teardown around each test?
There is a discussion about the need for a "manual" global `HTTPClient::initialize()` [[ https://reviews.llvm.org/D112753?id=385686#inline-1082736 | here ]]. Briefly, Curl's global initialization is not thread-safe and may load other libs; the loader lock on windows prevents use of a static ctor to initialize.  We added `HTTPClient::initialize()` to `InitLLVM` for convenience, but we had to remove it when HTTPClient got moved to the Debuginfod lib. So now the HTTPClient::initialize must be called "manually". By the way, there is [[ https://daniel.haxx.se/blog/2020/03/01/imagining-a-thread-safe-curl_global_init/ | ongoing discussion within libcurl community ]] on making curl global init thread safe.

On the other hand, we can safely deinitialize with a static destructor as you suggest.
This is a great idea which jhenderson also suggested on D113717, and you can see the implementation here:
https://reviews.llvm.org/D113717#change-LO0j60bMM9xj
So there is no need for the de-initialize / teardown here, pending that diff.


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

https://reviews.llvm.org/D114415