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

[Noah Shutty via Phabricator via llvm-commits]

noajshu added inline comments.


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:83
+  void *Curl = nullptr;
+  Error curlInit();
+  ~HTTPClient();
----------------
noajshu wrote:
> 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?
After some more looking, I think the [[ https://llvm.org/doxygen/classllvm_1_1ManagedStatic.html | ManagedStatic ]] template may be just what we want. I'm running some experiments to see if it will fit our needs here.


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

https://reviews.llvm.org/D112753