[PATCH] D112753: [llvm] [Support] Add CURL HTTP Client.

[Noah Shutty via Phabricator via llvm-commits]

noajshu marked an inline comment as not done.
noajshu added inline comments.


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:83
+  void *Curl = nullptr;
+  Error curlInit();
+  ~HTTPClient();
----------------
labath wrote:
> noajshu wrote:
> > Hi @labath, do you think we should follow the [[ https://llvm.org/docs/ProgrammersManual.html#fallible-constructors | style guide recommendation ]] for fallible constructors instead of using `curlInit`? That is, `HTTPClient` would have a private constructor and a `static Expected<HTTPClient> Create()`, which would just run the code inside `curlInit`.
> Umm... maybe?
> 
> I don't think that doing this would be a bad choice, but for me, the more interesting question is whether we should rely on the implicit `curl_global_init` call that happens as a part of this. This seems to be discouraged by curl, and this kind of initialization is not a particularly nice thing for a library (which llvm is) to do.
> 
> If we can guarantee that `curl_global_init` was called, then we might not even need a fallible constructor here. The man page is not particularly clear on this, but I would expect that the only reason this can fail in this case is memory (or some other soft resource) exhaustion, and llvm is not robust against those anyway...
Thank you so much for pointing out this issue.

Three solutions come to mind:

1. The elegant C++ solution:
```
// HTTPClient.cpp
class CurlGlobalState {
public:
  CurlGlobalState() {
    curl_global_init(CURL_GLOBAL_ALL);
  }
  ~CurlGlobalState() {
    curl_global_cleanup(CURL_GLOBAL_ALL);
  }
};
static const CurlGlobalState CurlState;
```
The only potential issue here is that a library user loading LLVM Support as a Windows DLL could hit a deadlock (noted [[ https://curl.se/libcurl/c/libcurl.html | here ]], more details [[ https://docs.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-best-practices | here ]]).

2. The less-elegant solution: exposing the global init and cleanup functions to the user, e.g. wrapped by:
```
static HTTPClient::globalInit();
static HTTPClient::globalCleanup();
```
and just asking users to call these at the beginning and end of the program.

3. The LLVM-ey compromise:
Throw the global init and cleanup into `InitLLVM` which [[ https://reviews.llvm.org/D45602 | seems to have been made ]] for purposes just like this.

Do you lean towards any particular solution?


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

https://reviews.llvm.org/D112753